[PATCH] D91589: [DAGCombiner] Fold (sext (not i1 x)) -> (add (zext i1 x), -1)

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:2325-2327
+        SDValue Not = DAG.getNOT(DL, X, X.getValueType());
+        if (SDValue Xor = visitXOR(Not.getNode()))
+          return DAG.getNode(ISD::SIGN_EXTEND, DL, VT, Xor);
----------------
laytonio wrote:
> spatel wrote:
> > We do not usually speculatively create nodes like that 'Not' end then not explicitly delete it if it wasn't necessary. 
> > 
> > It's not clear to me from the description or code comments why we are doing this transform at all. This is the opposite of the patch title? 
> I will update the patch to delete the Not node if not necessary.
> 
> As for why we are doing this, we would regress in some cases (eg in the sext_of_not_cmp tests added to RISCV and SystemZ) where we would have otherwise folded away the (not i1 x). In general as the TODO above indicates we should probably have a TLI method to determine whether we want to prefer the zext or sext versions of this and several other folds. It might make sense to use TLI.getBooleanContents for this, as we already do for the (add (sext i1 Y), X) -> (sub X, (zext i1 Y)) fold in visitADDLikeCommutative.
Yes - using getBooleanContents seems like a better option.
Another possibility is that we extend the pattern matching to include a `setcc` node (assuming that can absorb the `not` op).


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

https://reviews.llvm.org/D91589