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

[Thomas Lively via Phabricator via llvm-commits]

tlively added a comment.

Thanks for the patch! It looks like this will be a nice improvement.

@craig.topper, it would be great to get your comments as well, as someone more familiar with the target-independent infrastructure here.



================
Comment at: llvm/include/llvm/CodeGen/SelectionDAG.h:2459-2460
   DenseMap<MCSymbol *, SDNode *> MCSymbols;
+  DenseMap<const SDNode *, std::pair<const Value *, unsigned>>
+      ExportedSplatValueMap;
 
----------------
It would be good to add a comment describing the contents of this map.


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


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:844
+
+  return I->isShift() && isShiftAmountScalar() && I->getOperand(1) == Splat;
+}
----------------
What is the benefit of including `isShiftAmountScalar()` here, given that we know it is always true?


================
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
+
----------------
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.


================
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
----------------
Can we add a test where the vshift is a phi, just to show that that still works correctly?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158399