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

[Filipe Cabecinhas via Phabricator via llvm-commits]

filcab added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:876
 
+// Attempt to match aan unary predicate against a scalar/splat constant or
+// every element of a constant BUILD_VECTOR.
----------------
s/aan/a/


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:880
+                                std::function<bool(ConstantSDNode *)> Match) {
+  if (ConstantSDNode *Cst = dyn_cast<ConstantSDNode>(Op))
+    return Match(Cst);
----------------
`auto *Cst`


================
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;
----------------
Don't all the `BUILD_VECTOR` operands have to have the same type? If so, do we need `SVT`?
I might be missing something.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:917
+      return false;
+    if (LHSCst->getValueType(0) != SVT ||
+        LHSCst->getValueType(0) != RHSCst->getValueType(0))
----------------
Is it possible that this succeeds?


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:918
+    if (LHSCst->getValueType(0) != SVT ||
+        LHSCst->getValueType(0) != RHSCst->getValueType(0))
+      return false;
----------------
`getValueType()` returns the same for `LHS` and `RHS`. Is it possible for this test to succeed?


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:5402
+  };
+  if (matchUnaryPredicate(N1, MatchShiftTooBig))
     return DAG.getUNDEF(VT);
----------------
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.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:5438
+    if (matchBinaryPredicate(N1, N0.getOperand(1), MatchOutOfRange))
+      return DAG.getConstant(0, SDLoc(N), VT);
 
----------------
Re: comment above: This one is ok, as we'll only be able to replace all with zeroes when all the "sums of shift amounts" are out of range.


Repository:
  rL LLVM

https://reviews.llvm.org/D35492