[PATCH] D91589: [DAGCombiner] Fold (sext (not i1 x)) -> (add (zext i1 x), -1)
Layton Kifer via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Nov 24 21:38:45 PST 2020
laytonio 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);
> spatel wrote:
> > 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).
> I tried an implementation using getBooleanContents here and in a few other places, and it seems promising, but there are a few regressions I was having a hard time nailing down. I would like to save this for a follow up patch if possible.
> While inverting a `setcc` is probably the most common way we would be able to absorb the `not`, I'm don't know if it makes sense to only check that case. At the least, I think `(not (or x, y)) -> (and (not x), (not y)) iff x or y are setcc` could also be useful.
Using isBitwiseNot below as suggested caused us to regress in a new case. Upon investigating, I've realized my argument for using visitXOR here is incorrect. At the time we try to fold away the created `not`, the value we're trying to fold the `not` into has two uses, the new `not`, and the original `zext`. This causes most of the folds in visitXOR not to fire. I'm not sure there is a good way to overcome this. I believe conditioning the initial fold on having a non-foldable `not` is probably the better solution.
CHANGES SINCE LAST ACTION
More information about the llvm-commits