This is an archive of the discontinued LLVM Phabricator instance.

[SelectionDAG] Merge FoldConstantVectorArithmetic into FoldConstantArithmetic (PR36544)
ClosedPublic

Authored by RKSimon on Nov 5 2021, 10:46 AM.

Details

Summary

This patch merges FoldConstantVectorArithmetic back into FoldConstantArithmetic.

Like FoldConstantVectorArithmetic we now handle vector ops with any operand count, but we currently still only handle binops for scalar types - this can be improved in future patches - in particular some common unary/trinary ops still have poor constant folding.

There's one change in functionality causing test changes - FoldConstantVectorArithmetic bails early if the build/splat vector isn't all constant (with some undefs) elements, but FoldConstantArithmetic doesn't - it instead attempts to fold the scalar nodes and bails if they fail to regenerate a constant/undef result, allowing some additional identity/undef patterns to be handled.

Diff Detail

Event Timeline

RKSimon created this revision.Nov 5 2021, 10:46 AM
RKSimon requested review of this revision.Nov 5 2021, 10:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 5 2021, 10:46 AM
Herald added a subscriber: MaskRay. · View Herald Transcript
RKSimon retitled this revision from [SelectionDAG] Merge FoldConstantVectorArithmetic into FoldConstantArithmetic to [SelectionDAG] Merge FoldConstantVectorArithmetic into FoldConstantArithmetic (PR36544).Nov 5 2021, 10:52 AM

Hi @RKSimon ,
First, let me apologize as I completely forgot the original bug was assigned to me. Thank you for tackling this.

This LGTM (pending the fix to the DAGCombiner.cpp file) but I'm no expert so another review would be ideal. I left a couple comments that are more for my own understanding rather than actual review comments :)

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
5319

Question: Are we just removing the need for the build/splat vector to be constant here because it's not needed? If so, I wonder why it was enforced in the first place?

llvm/test/CodeGen/RISCV/urem-seteq-illegal-types.ll
644

Question: This is a bit of an open ended question for my own learning, but how did you know what to update these tests to? I understand the updating of the SelectionDag code, but have a hard time understanding how you figured out what to update these tests to so that they pass.

lebedev.ri added inline comments.Nov 5 2021, 1:10 PM
llvm/test/CodeGen/RISCV/urem-seteq-illegal-types.ll
644

See first line of the file:

; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py

Hi @RKSimon ,
First, let me apologize as I completely forgot the original bug was assigned to me. Thank you for tackling this.

No worries - I should have pinged you, but this only came about today because I'm trying to extend constant folding to handle cases where it needs to peek through bitcasts (beyond what I've done in D113202), and getting this dealt with first made sense.

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
5319

As I said in the summary - FoldConstantArithmetic doesn't bail out for non-constant source data, meaning we pick up a few identity (sub x, x) and undef style cases that we missed if we only ever called FoldConstantVectprArithmetic.

We still bail if the result isn't constant/undef of course.

llvm/test/CodeGen/RISCV/urem-seteq-illegal-types.ll
644

Yes, I just ran the update_llc_test_checks.py script - its been a massive time saver since we got those scripts, and has meant that we have much more complete checks, which has helped prevent a lot of regressions in code that we weren't (manually) checking properly.

craig.topper added inline comments.Nov 5 2021, 1:34 PM
llvm/test/CodeGen/RISCV/urem-seteq-illegal-types.ll
572

Is this just the same instructions in a different order?

RKSimon added inline comments.Nov 5 2021, 2:35 PM
llvm/test/CodeGen/RISCV/urem-seteq-illegal-types.ll
572

Yes it appears to be the same instructions, just reordered and with slightly different regalloc.

lebedev.ri accepted this revision.Nov 8 2021, 9:48 AM

Seems fine to me, but best to wait for one more review.

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
5295–5306

I think this should be in the previous if-block (2 ops)

This revision is now accepted and ready to land.Nov 8 2021, 9:48 AM
This revision was landed with ongoing or failed builds.Nov 9 2021, 3:31 AM
This revision was automatically updated to reflect the committed changes.