[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 :
----------------
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.)


================
Comment at: lib/Target/WebAssembly/WebAssemblyInstrSIMD.td:169
+}
+multiclass SIMDCondition<string name, string cond, bits<32> baseInst> {
+ defm _S : SIMDConditionInt<name#"_s", !cast<CondCode>("SET"#cond),
----------------
I can see why you added this class, but that some instructions use this and the others bypasses this and directly use `SIMDConditionInt` and `SIMDConditionFP` directly looks a bit confusing to me. Even if we delete this and manually add `_u` instructions below, it's gonna be only (?) +8 lines, so I guess it'd be easier to read that way..?


================
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
----------------
How about renaming this to `simd-comparisons.ll` to match other file name conventions you've added so far?


Repository:
  rL LLVM

https://reviews.llvm.org/D51765