[PATCH] D60020: [DAGCombiner][X86][SystemZ] Canonicalize SSUBO with immediate RHS to SADDO by negating the immediate.

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:3047
+  // fold (subox, c) -> (addo x, -c)
+  if (IsSigned && N1C && !N1C->getAPIntValue().isMinSignedValue()) {
+    return DAG.getNode(ISD::SADDO, DL, N->getVTList(), N0,
----------------
craig.topper wrote:
> dlrobertson wrote:
> > I don't think this path would be hit post-D60061 right? Would this be for the case that `opt` is run without `instcombine`?
> > 
> > Also D60061 uses `Constant` and `isNotMinSignedValue`. Is there a reason not to use `N1C->getConstantIntValue()->isNotMinSignedValue` here too? Are there pros/cons to either one?
> Not sure if this can come up in real code after D60061, but we have lit tests here that change. Should we update those to post D60061 code instead?
> 
> IsNotMinSignedValue does dyn_cast checks for ConstantInt, ConstantFP, and vectors. Seems like a bit of wasted work if we already know we're looking at an integer.
ssubo nodes may be created during ssubsat legalization, so it's possible for nodes of this form to occur during codegen of canonical IR, even after D60061. With that in mind, I think it's okay to have this as a DAG transform as well.

isNotMinSignedValue() isn't applicable here as it operates on IR constants. We could replicate the same effect using matchUnaryPredicate() here, if we're interested in parity.


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

https://reviews.llvm.org/D60020