This is an archive of the discontinued LLVM Phabricator instance.

[SDAG] Remove single-result restriction on commutative CSE
ClosedPublic

Authored by ibookstein on Jul 13 2022, 10:36 AM.

Details

Summary

The DAG Combiner unnecessarily restricts commutative CSE
to nodes with a single result value. This commit removes
that restriction.

Signed-off-by: Itay Bookstein <ibookstein@gmail.com>

Diff Detail

Event Timeline

ibookstein created this revision.Jul 13 2022, 10:36 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 13 2022, 10:36 AM
ibookstein requested review of this revision.Jul 13 2022, 10:36 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 13 2022, 10:36 AM
craig.topper added inline comments.Jul 13 2022, 10:47 AM
llvm/test/CodeGen/X86/dagcombine-multiple-results.ll
1 ↗(On Diff #444333)

pre-commit this test?

RKSimon added inline comments.Jul 13 2022, 10:50 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
1872

Do we know why this limit was here in the first place?

craig.topper added inline comments.Jul 13 2022, 10:59 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
1872

At the time that was added, the commutable binops with 2 values were SMUL_LOHI/UMUL_LOHI/ADDC/ADDE.

Am I unsure if its safe to do this for ADDC/ADDE which use Glue results. CSEing would increase the use count of the Glue. Maybe that's what it was protecting?

1872

Oops "Am I unsure" -> "I am unsure".

craig.topper added inline comments.Jul 13 2022, 11:07 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
1872

So it's probably safer as

if (!RV.getNode() && TLI.isCommutativeBinOp(N->getOpcode()) &&
    N->getValueType(N->getNumValues()-1) != MVT::Glue) {
barannikov88 added inline comments.
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
1872

Am I unsure if its safe to do this for ADDC/ADDE which use Glue results. CSEing would increase the use count of the Glue.

AFAIK nodes with Glue result are never CSEd. See getNode getNodeIfExists.

craig.topper added inline comments.Jul 13 2022, 11:31 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
1872

Ok then I have no idea what the getNumValues() was protecting. The commit that added it is the same commit that added SelectionDAG::getNodeIfExists including the Glue check inside of it.

barannikov88 added inline comments.Jul 13 2022, 11:32 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
1872

Sorry, wrong links
getNode
getNodeIfExists
There is also doNotCSE

Rebased on top of a commit that separately adds the CSE test.

RKSimon accepted this revision.Jul 18 2022, 1:54 AM

LGTM - cheers

This revision is now accepted and ready to land.Jul 18 2022, 1:54 AM
This revision was landed with ongoing or failed builds.Jul 18 2022, 9:19 AM
This revision was automatically updated to reflect the committed changes.