[PATCH] D93771: [WebAssembly][NFC] Finish cleaning up SIMD tablegen

[Thomas Lively via Phabricator via llvm-commits]

tlively added inline comments.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrSIMD.td:63
   let vt = v16i8;
+  let int_vt = v16i8;
   let lane_vt = i32;
----------------
aheejin wrote:
> Can we do this? (Other places too)
Yes, it turns out we can!


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrSIMD.td:508
+            imm:$m8, imm:$m9, imm:$mA, imm:$mB,
+            imm:$mC, imm:$mD, imm:$mE, imm:$mF)>;
 }
----------------
aheejin wrote:
> Does not using `LaneIdx32` change anything? (I guess `LaneIdx` has range checks but this doesn't?) (By the way it looks it should be `LaneIdx16`)
If I understand correctly, using `LaneIdx32` in the input pattern is sufficient to apply the range checks, so it's not needed in the output pattern as well. (However, `imm` is still needed because it makes the resulting MachineOperand an immediate rather than a register.)

Shuffle uses `LaneIdx32` because the indices can refer to bytes in either of the two input vectors.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrSIMD.td:721
+                   "v128."#name#"\t$dst, $lhs, $rhs", "v128."#name, simdop>;
+  foreach vec = [I8x16, I16x8, I32x4, I64x2] in
+  def : Pat<(node (vec.vt V128:$lhs), (vec.vt V128:$rhs)),
----------------
aheejin wrote:
> How about defining something like
> ```
> defvar IntVecs = [I8x16, I16x8, I32x4, I64x2];
> ```
> at the top and use it elsewhere?
Good idea, done.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrSIMD.td:724
+            (!cast<NI>(NAME) $lhs, $rhs)>;
 }
 
----------------
aheejin wrote:
> Any reason `SIMDBitwise` does not use `SIMDBinary` anymore?
The previous version used `SIMDBitwise` created four different version of the same instruction when all we really needed was one instruction with four different patterns. Since it turns out we can use `foreach` in a multiclass to generate `Pat`s and use `NAME` to refer to the instruction in those `Pat`s, this seemed like a good way to get rid of the redundant instruction definitions.

This also lets `SIMDBinary` get rid of the `prefix` argument since `SIMDBinary` no longer needs to generate instructions that start with "v128."


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrSIMD.td:735-738
+defm NOT : SIMD_I<(outs V128:$dst), (ins V128:$v), (outs), (ins), [],
+                  "v128.not\t$dst, $v", "v128.not", 77>;
+foreach vec = [I8x16, I16x8, I32x4, I64x2] in
+def : Pat<(vnot (vec.vt V128:$v)), (NOT $v)>;
----------------
aheejin wrote:
> Any reason `NOT` does not use `SIMDUnary`?
Similar to the reasons for the previous change. We don't need four different versions of `NOT` and this lets `SIMDUnary` get rid of the prefix argument.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrSIMD.td:1120
 
-multiclass SIMDWiden<ValueType vec_t, string vec, ValueType arg_t, string arg,
-                     bits<32> baseInst> {
-  defm "" : SIMDConvert<vec_t, arg_t, widen_low_s,
-                        vec#".widen_low_"#arg#"_s", baseInst>;
-  defm "" : SIMDConvert<vec_t, arg_t, widen_high_s,
-                        vec#".widen_high_"#arg#"_s", !add(baseInst, 1)>;
-  defm "" : SIMDConvert<vec_t, arg_t, widen_low_u,
-                        vec#".widen_low_"#arg#"_u", !add(baseInst, 2)>;
-  defm "" : SIMDConvert<vec_t, arg_t, widen_high_u,
-                        vec#".widen_high_"#arg#"_u", !add(baseInst, 3)>;
+// TODO: refactor this to be uniform for i64x2 if the numbering is not changed.
+multiclass SIMDWiden<Vec vec, bits<32> baseInst> {
----------------
aheejin wrote:
> Is it subject to change?
These opcodes are not part of a consistent design, but rather chosen from the available opcode space to get prototypes done in V8 as quickly as possible. I'll be a little sad if the opcodes we end up shipping don't have a consistent arrangement, but we haven't discussed whether we are going to do another change yet, so I don't know whether this will change.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrSIMD.td:1145-1157
+multiclass SIMDNarrow<Vec vec, bits<32> baseInst> {
+  defvar name = vec.split.prefix#".narrow_"#vec.prefix;
+  defm NARROW_S_#vec.split :
     SIMD_I<(outs V128:$dst), (ins V128:$low, V128:$high), (outs), (ins),
-           [(set (vec_t V128:$dst), (vec_t (int_wasm_narrow_signed
-             (arg_t V128:$low), (arg_t V128:$high))))],
-           vec#".narrow_"#arg#"_s\t$dst, $low, $high", vec#".narrow_"#arg#"_s",
-           baseInst>;
-  defm NARROW_U_#vec_t :
+           [(set (vec.split.vt V128:$dst), (vec.split.vt (int_wasm_narrow_signed
+             (vec.vt V128:$low), (vec.vt V128:$high))))],
+           name#"_s\t$dst, $low, $high", name#"_s", baseInst>;
----------------
aheejin wrote:
> Any reason this does not use `SIMDConvert`?
Yes, these narrowing operations take two operands but `SIMDConvert` instructions have just one operand.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93771