This happens when CMP1 and CMP3 have the same predicate (or CMP2 and CMP3 have
the same predicate).
This helps optimizations such as the fololowing one:
CMP(A,C)||CMP(B,C) => CMP(MIN/MAX(A,B), C)
CMP(A,C)&&CMP(B,C) => CMP(MIN/MAX(A,B), C)
Paths
| Differential D156215
[DAGCombiner] Reassociate the operands from (OR (OR(CMP1, CMP2)), CMP3) to (OR (OR(CMP1, CMP3)), CMP2) ClosedPublic Authored by kmitropoulou on Jul 25 2023, 1:52 AM.
Details Summary This happens when CMP1 and CMP3 have the same predicate (or CMP2 and CMP3 have This helps optimizations such as the fololowing one:
Diff Detail
Event TimelineComment Actions Updating D156215: [DAGcombiner] Reassociate the operands from (OR (OR(CMP1, CMP2)), CMP3) to (OR (OR(CMP1, CMP3)), CMP2) Comment Actions Updating D156215: [DAGcombiner] Reassociate the operands from (OR (OR(CMP1, CMP2)), CMP3) to (OR (OR(CMP1, CMP3)), CMP2) kmitropoulou added a parent revision: D155915: [NFC][DAGCombiner] Tests for future commit..Jul 25 2023, 1:58 AM Comment Actions Updating D156215: [DAGcombiner] Reassociate the operands from (OR (OR(CMP1, CMP2)), CMP3) to (OR (OR(CMP1, CMP3)), CMP2)
kmitropoulou marked 2 inline comments as done. Comment ActionsUpdating D156215: [DAGcombiner] Reassociate the operands from (OR (OR(CMP1, CMP2)), CMP3) to (OR (OR(CMP1, CMP3)), CMP2) kmitropoulou retitled this revision from [DAGcombiner] Reassociate the operands from (OR (OR(CMP1, CMP2)), CMP3) to (OR (OR(CMP1, CMP3)), CMP2) to [DAGCombiner] Reassociate the operands from (OR (OR(CMP1, CMP2)), CMP3) to (OR (OR(CMP1, CMP3)), CMP2).Jul 25 2023, 12:02 PM Comment Actions Updating D156215: [DAGCombiner] Reassociate the operands from (OR (OR(CMP1, CMP2)), CMP3) to (OR (OR(CMP1, CMP3)), CMP2) Comment Actions Updating D156215: [DAGCombiner] Reassociate the operands from (OR (OR(CMP1, CMP2)), CMP3) to (OR (OR(CMP1, CMP3)), CMP2) Comment Actions Seems reasonable, but I wonder how often this helps in real code. Your patch does not handle swapped comparison conditions, e.g. reassociating an expression so that a<b can be combined with b>c. Doing that properly would make it much more complicated and I don't think it is worth it. Your patch does not help if there is a tree of ANDs or ORs with more than three setcc nodes.
Comment Actions
I ran the Vulcan game tests. The reassociateOpsCommutative() is called 115168 times and the proposed optimization is triggered 302 times. So, I will abandon the patch. Comment Actions
That seems worthwhile? Why abandon it? Comment Actions
I am sorry I restored it. Comment Actions Updating D156215: [DAGCombiner] Reassociate the operands from (OR (OR(CMP1, CMP2)), CMP3) to (OR (OR(CMP1, CMP3)), CMP2) This revision is now accepted and ready to land.Aug 8 2023, 1:44 PM Comment Actions Updating D156215: [DAGCombiner] Reassociate the operands from (OR (OR(CMP1, CMP2)), CMP3) to (OR (OR(CMP1, CMP3)), CMP2) Closed by commit rG2c5d1b5ab703: [DAGCombiner] Reassociate the operands from (OR (OR(CMP1, CMP2)), CMP3) to (OR… (authored by kmitropoulou). · Explain WhyAug 8 2023, 8:08 PM This revision was automatically updated to reflect the committed changes.
Revision Contents
Diff 543872 llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
llvm/test/CodeGen/AMDGPU/combine_andor_with_cmps.ll
llvm/test/CodeGen/AMDGPU/wave32.ll
llvm/test/CodeGen/Hexagon/isel/logical.ll
llvm/test/CodeGen/X86/v8i1-masks.ll
|
Typo "N001"