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

[Heejin Ahn via Phabricator via llvm-commits]

aheejin added a comment.

Nice!



================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrSIMD.td:63
   let vt = v16i8;
+  let int_vt = v16i8;
   let lane_vt = i32;
----------------
Can we do this? (Other places too)


================
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)>;
 }
----------------
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`)


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrSIMD.td:525-529
+                             (outs), (ins),
+                             [(set (vec.vt V128:$dst),
+                                (vec.splat vec.lane_rc:$x))],
+                             vec.prefix#".splat\t$dst, $x", vec.prefix#".splat",
+                             simdop>;
----------------



================
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)),
----------------
How about defining something like
```
defvar IntVecs = [I8x16, I16x8, I32x4, I64x2];
```
at the top and use it elsewhere?


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrSIMD.td:724
+            (!cast<NI>(NAME) $lhs, $rhs)>;
 }
 
----------------
Any reason `SIMDBitwise` does not use `SIMDBinary` anymore?


================
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)>;
----------------
Any reason `NOT` does not use `SIMDUnary`?


================
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> {
----------------
Is it subject to 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>;
----------------
Any reason this does not use `SIMDConvert`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93771