[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:2329
+
+        recursivelyDeleteUnusedNodes(Not.getNode());
+      }
----------------
Please add a code comment with something like this:
  // The speculatively created 'not' node could not be folded away, 
  // so delete it immediately to avoid conflicting with other folds.  


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:10586
+  if (N0.getOpcode() == ISD::XOR && N0.hasOneUse() &&
+      N0.getScalarValueSizeInBits() == 1 && isOneOrOneSplat(N0.getOperand(1)) &&
+      (!LegalOperations || (TLI.isOperationLegal(ISD::ZERO_EXTEND, VT) &&
----------------
Can we use this here:
  /// Returns true if \p V is a bitwise not operation. Assumes that an all ones
  /// constant is canonicalized to be operand 1.
  bool isBitwiseNot(SDValue V, bool AllowUndefs = false);



================
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.
----------------
laytonio wrote:
> spatel wrote:
> > Shouldn't that "zext" be "sext"? We are creating sext in this block.
> No, because the sext is only better if we can fold away the not. As the comment below indicates. The actual fold that creates the zext of this form is not here, but in visitSIGN_EXTEND.
Ok - I understand now, but I think we can improve the order of the statements to make it clearer. 
How about:
    // add (zext i1 X), -1 -> sext (not i1 X)
    // Usually, we transform this pattern in the opposite 
    // direction because most targets generate better code for
    // the zext form. However, if we can eliminate the 'not', the
    // sext form should be better.
    // TODO: ... 


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

https://reviews.llvm.org/D91589