[PATCH] D35492: [DAGCombiner] Match non-uniform constant vectors using predicates (RFC).

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:889
+    auto *Cst = dyn_cast<ConstantSDNode>(Op.getOperand(i));
+    if (!Cst || Cst->getValueType(0) != SVT || !Match(Cst))
+      return false;
----------------
filcab wrote:
> Don't all the `BUILD_VECTOR` operands have to have the same type? If so, do we need `SVT`?
> I might be missing something.
BUILD_VECTOR operands do all have to have the same type, but implicit truncation to the vector's scalar type is permitted which can make things tricky - see isConstOrConstSplat which checks for the same thing.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:918
+    if (LHSCst->getValueType(0) != SVT ||
+        LHSCst->getValueType(0) != RHSCst->getValueType(0))
+      return false;
----------------
filcab wrote:
> `getValueType()` returns the same for `LHS` and `RHS`. Is it possible for this test to succeed?
Again, implicit truncation in the BUILD_VECTORs, and since we have 2 separate BUILD_VECTORs they may be doing different things.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:5402
+  };
+  if (matchUnaryPredicate(N1, MatchShiftTooBig))
     return DAG.getUNDEF(VT);
----------------
filcab wrote:
> This looks different than what I expected.
> What's the behaviour you want?
>   - If *one* shift is too big, it triggers UB, so replace with undef
>   - or, if *all* the shifts are too big, replace with undef as there's not even one "sane" value
> 
> If you want the first (which I'm assuming we'd like, as we'd expose more optimizations), shouldn't you be doing something like this?
> `if (!matchUnaryPredicate(N1, MatchAllIsOK)) return undef;`
> 
> Of course, you might prefer the second, but in that case I'd ask for a small comment on why. Either in code or in the commit message.
This just covers the 'all shifts are known constants and too big' case which is about as much as we can dare to support here - note that DAGCombine doesn't handle 'fold (shl x, undef)'.

I suppose we could have a 'matchAnyUnaryPredicate' version that returns true if any case matches but I don't yet have a use case.


Repository:
  rL LLVM

https://reviews.llvm.org/D35492