[PATCH] D129666: [SDAG] Remove single-result restriction on commutative CSE

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:1872
-  if (!RV.getNode() && TLI.isCommutativeBinOp(N->getOpcode()) &&
-      N->getNumValues() == 1) {
     SDValue N0 = N->getOperand(0);
----------------
barannikov88 wrote:
> craig.topper wrote:
> > craig.topper wrote:
> > > craig.topper wrote:
> > > > RKSimon wrote:
> > > > > Do we know why this limit was here in the first place?
> > > > 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?
> > > Oops "Am I unsure" -> "I am unsure".
> > So it's probably safer as
> > 
> > ```
> >   if (!RV.getNode() && TLI.isCommutativeBinOp(N->getOpcode()) &&
> >       N->getValueType(N->getNumValues()-1) != MVT::Glue) {
> > ```
> > 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 [[ https://github.com/s-barannikov/llvm-neuromatrix/blob/ede600377cb6df1bef71f070130d8cfe734cc5b7/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp#L8984 | getNode ]] [[ https://github.com/s-barannikov/llvm-neuromatrix/blob/ede600377cb6df1bef71f070130d8cfe734cc5b7/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp#L9722 | getNodeIfExists ]].
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129666/new/

https://reviews.llvm.org/D129666