[PATCH] D158399: [WebAssembly] Optimize vector shift using a splat value from outside block

[Thomas Lively via Phabricator via llvm-commits]

tlively added inline comments.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:840
+    const Instruction *I, const Value *Splat) const {
+  if (!I) {
+    return isShiftAmountScalar();
----------------
YolandaCY wrote:
> tlively wrote:
> > Why allow null instruction pointers here? It would seem simpler to ensure that callers pass a valid instruction and to assume we have a valid instruction here.
> This is a fast check on the outside block when visit the splat vector, and don't know yet if the splat vector will be used by a shift op. 
> To identify the instruction I need to iterate all uses of the splat vector until we find the vector shift. Since this is only needed for WebAssembly target, I add a quick check here to reduce the cost for other platforms. Seems a little confusion, do you think we need to seperate it to two functions? 
Oh I see, that makes sense.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:844
+
+  return I->isShift() && isShiftAmountScalar() && I->getOperand(1) == Splat;
+}
----------------
YolandaCY wrote:
> tlively wrote:
> > What is the benefit of including `isShiftAmountScalar()` here, given that we know it is always true?
> This will be called in SelectionDAGBuilder to skip the optimizaiton for other platforms when visit shift.
We know that the `isShiftAmountScalar()` here will be the WebAssemblyTargetLowering version of `isShiftAmountScalar()`, so it will always be true, right? So this line could be:

`return I->isShift() && I->getOperand(1) == Splat;`



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158399