This is an archive of the discontinued LLVM Phabricator instance.

[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
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)

Diff Detail

Event Timeline

kmitropoulou created this revision.Jul 25 2023, 1:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 25 2023, 1:52 AM
kmitropoulou requested review of this revision.Jul 25 2023, 1:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 25 2023, 1:52 AM

Updating D156215: [DAGcombiner] Reassociate the operands from (OR (OR(CMP1, CMP2)), CMP3) to (OR (OR(CMP1, CMP3)), CMP2)

kmitropoulou abandoned this revision.Jul 25 2023, 1:57 AM

Updating D156215: [DAGcombiner] Reassociate the operands from (OR (OR(CMP1, CMP2)), CMP3) to (OR (OR(CMP1, CMP3)), CMP2)

Updating D156215: [DAGcombiner] Reassociate the operands from (OR (OR(CMP1, CMP2)), CMP3) to (OR (OR(CMP1, CMP3)), CMP2)

kmitropoulou added inline comments.Jul 25 2023, 2:07 AM
llvm/test/CodeGen/Hexagon/isel/logical.ll
1253

One of the comparisons is ne when we process the nodes of the DAG.

arsenm added inline comments.Jul 25 2023, 9:04 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
1338–1341

Should add a dag comment explaining the transform

1347

no else after return

kmitropoulou marked 2 inline comments as done.

Updating 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

Updating D156215: [DAGCombiner] Reassociate the operands from (OR (OR(CMP1, CMP2)), CMP3) to (OR (OR(CMP1, CMP3)), CMP2)

Updating D156215: [DAGCombiner] Reassociate the operands from (OR (OR(CMP1, CMP2)), CMP3) to (OR (OR(CMP1, CMP3)), CMP2)

foad added a comment.Jul 27 2023, 3:17 AM

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.

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
1338

Typo "N001"

kmitropoulou added a comment.EditedAug 1 2023, 9:45 PM

Seems reasonable, but I wonder how often this helps in real code.

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.

kmitropoulou abandoned this revision.Aug 1 2023, 9:45 PM
arsenm added a comment.Aug 2 2023, 3:17 PM

Seems reasonable, but I wonder how often this helps in real code.

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.

That seems worthwhile? Why abandon it?

kmitropoulou reclaimed this revision.Aug 2 2023, 4:05 PM

Seems reasonable, but I wonder how often this helps in real code.

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.

That seems worthwhile? Why abandon it?

I am sorry I restored it.

Updating D156215: [DAGCombiner] Reassociate the operands from (OR (OR(CMP1, CMP2)), CMP3) to (OR (OR(CMP1, CMP3)), CMP2)

arsenm accepted this revision.Aug 8 2023, 1:44 PM
This revision is now accepted and ready to land.Aug 8 2023, 1:44 PM

Updating D156215: [DAGCombiner] Reassociate the operands from (OR (OR(CMP1, CMP2)), CMP3) to (OR (OR(CMP1, CMP3)), CMP2)