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

[Layton Kifer via Phabricator via llvm-commits]

laytonio added a comment.

In D91589#2408627 <https://reviews.llvm.org/D91589#2408627>, @spatel wrote:

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

I don't believe this is a danger because going from (sext (not i1 x)) to (add (zext i1 x), -1) requires (not i1 x), and we only go back when we immediately fold away the created (not i1 x). We could instead decide not to do the initial fold if we have a (not i1 x), but if we fold to (add (zext i1 x), -1) first we have the opportunity to constant fold the -1 away, and additionally cover cases where we initially have the (add (zext i1 x), -1) version. (eg in the dec_of_zexted_cmp tests added to RISCV and SystemZ)



================
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.
----------------
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.


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


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

https://reviews.llvm.org/D91589