[PATCH] D51765: [WebAssembly] SIMD comparisons

[Heejin Ahn via Phabricator via llvm-commits]

aheejin added inline comments.


================
Comment at: lib/Target/WebAssembly/WebAssemblyInstrSIMD.td:149
+multiclass SIMDConditionImpl<ValueType vec_t, ValueType out_t, string vec,
+                             string name, CondCode cond, bits<32> simdop> {
+  defm _#vec_t :
----------------
tlively wrote:
> aheejin wrote:
> > When do you use `simdop` and when do you use `baseInst`? Do you use it interchangeably? If so we should use the same variable name I guess. (In previous patches too.)
> I try to use `baseInst` when the multiclass will add constants to it to create new opcodes and  `simdop` when it will be used as-is. I will go through and make sure this usage is consistent. I would also be fine with changing them all to `simdop` if you think that would make more sense.
Oh, I see. The usage makes sense I think.


================
Comment at: test/CodeGen/WebAssembly/comparisons_simd.ll:1
+; RUN: llc < %s -disable-wasm-fallthrough-return-opt -wasm-keep-registers -wasm-disable-explicit-locals -wasm-enable-unimplemented-simd -mattr=+simd128,+sign-ext --show-mc-encoding | FileCheck %s --check-prefixes CHECK,SIMD128
+; RUN: llc < %s -disable-wasm-fallthrough-return-opt -wasm-keep-registers -wasm-disable-explicit-locals -mattr=+simd128,+sign-ext --show-mc-encoding | FileCheck %s --check-prefixes CHECK,SIMD128-VM
----------------
tlively wrote:
> aheejin wrote:
> > How about renaming this to `simd-comparisons.ll` to match other file name conventions you've added so far?
> I named it this way to be consistent with `comparisons_f32.ll`, `comparisons_i32.ll` and friends. Those files should probably be renamed for consistency in a separate CL as well. What do you think of `comparisons-i32.ll`, `comparisons-simd.ll`, etc?
Hmm, yeah, they look like the only ones with underscores in file names. I think renaming them also is a good idea. But other SIMD-related test files all start with `simd-`, like `simd-arith.ll` or `simd-conversions.ll`, so I guess putting `simd` first makes more sense. Wait, there's an exception, there are exceptions that start with `offset-`. :facepalm: But anyway.


Repository:
  rL LLVM

https://reviews.llvm.org/D51765