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

[Yolanda via Phabricator via llvm-commits]

YolandaCY marked 3 inline comments as done.
YolandaCY added a comment.

Thank you Thomas for the comments! Please see my updates in the new revision.



================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:840
+    const Instruction *I, const Value *Splat) const {
+  if (!I) {
+    return isShiftAmountScalar();
----------------
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? 


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:844
+
+  return I->isShift() && isShiftAmountScalar() && I->getOperand(1) == Splat;
+}
----------------
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.


================
Comment at: llvm/test/CodeGen/WebAssembly/simd-shift-in-loop.ll:1
+; RUN: llc < %s -asm-verbose=false -verify-machineinstrs -disable-wasm-fallthrough-return-opt -wasm-disable-explicit-locals -wasm-keep-registers -mattr=+simd128 | FileCheck %s
+
----------------
tlively wrote:
> What do you think about using the auto-update script for this test? The output would be more verbose, but it would also be easy to update if anything changes, and I think it would be helpful to see that the whole function is emitted correctly.
Sure. Previously I make it simple to avoid mismatch on unrelated changes. But if we have an auto-update script that would be helpful to verify the whole function directly.


================
Comment at: llvm/test/CodeGen/WebAssembly/simd-shift-in-loop.ll:16
+ %t1 = insertelement <16 x i8> undef, i8 %shift, i32 0
+ %vshift = shufflevector <16 x i8> %t1, <16 x i8> undef, <16 x i32> zeroinitializer
+ br label %body
----------------
tlively wrote:
> Can we add a test where the vshift is a phi, just to show that that still works correctly?
Sure. I have added one more test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158399