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

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

I might have missed something - how are we guarding against these 2 transforms getting into an infinite loop?



================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:2313
+    // in the opposite direction because most targets generate better code for
+    // the zext form.
+    // TODO: Add a TLI method if any target prefers the opposite fold.
----------------
Shouldn't that "zext" be "sext"? We are creating sext in this block.


================
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);
----------------
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? 


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

https://reviews.llvm.org/D91589