[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
Mon Nov 23 03:50:53 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);
> 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.
CHANGES SINCE LAST ACTION
More information about the llvm-commits