[WebAssembly] Gate i64x2 and f64x2 on -wasm-enable-unimplemented

[tlively]

Summary

i64x2 and f64x2 operations are not implemented in V8, so we normally
do not want to emit them. However, they are in the SIMD spec proposal,
so we still want to be able to test them in the toolchain. This patch
adds a flag to enable their emission.

[dschuff]

lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp

45 ↗

(On Diff #159634)

Could putting this declaration in WebAssemblyTargetMachine.h avoid all these extern declarations?

[tlively]

lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp

45 ↗

(On Diff #159634)

Not all, just some.

[aheejin]

lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp

49 ↗

(On Diff #159634)

Are we planning to use this flag only for simd or also for other instructions? For example, exception handling is not currently implemented in the VM yet, but we are adding opcodes for it (under -mattr=+exception-handling flag). And in case if other stage 0~1 proposals come up, we may want to add them in the toolchain before they are implemented in the VM. Are we gonna use this flag for all these cases? If not, how about changing it to wasm-enable-unimplemented-simd?

test/CodeGen/WebAssembly/simd-arith.ll

11

How about leaving the meaning of SIMD128 as is and create another check mode for SIMD without the two opcodes? That way we can only delete that when we have the remaining two in the VM. And I think that also makes more sense if we are eventually gonna have them in the VM.

[tlively]

lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp

45 ↗

(On Diff #159634)

It also doesn't work that way because you end up with duplicate symbols.

[aheejin]

lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp

45 ↗

(On Diff #159634)

I guess what he meant is putting the definition where it is now and putting that extern declaration in the header file, but anyway.

[tlively]

lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp

49 ↗

(On Diff #159634)

It may make sense to use the same flag for all cases where ops are implemented in the toolchain but not the VM. However, I'd be happy to change the name if this will only be used for SIMD. Would you want to have a single general flag for this use case, or are other existing mechanisms sufficient?

[tlively]

• Update FileCheck prefixes

[aheejin]

lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp

49 ↗

(On Diff #159634)

I don't think we need a separate flag that handles all unimplemented opcodes because we already have feature flags for each not-fully-implemented-and-ongoing feature. If we have enough cases like this, where a feature is controlled by a feature flag but we need a separation even within it, using this flag for all those cases might make sense. But before that, I guess making it clear that this applies only for SIMD is less confusing, because currently it sounds like it encompasses all unimplemented features.

[aheejin]

lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp

45 ↗

(On Diff #159634)

Would WebAssembly.h work then?

[tlively]

• Change flag name, move extern decl to header

[tlively]

Addressed comments

[aheejin]

lib/Target/WebAssembly/WebAssembly.h

21 ↗

(On Diff #159788)

Hmm, I'm not sure if we want to include this big header (and all other headers included by it) into all WebAssembly cpp files. Sorry. Maybe the original way would be better..?

lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp

49 ↗

(On Diff #159634)

Nit: can you also change the variable name to include simd?

[tlively]

• Restore individual extern decls

[tlively]

• Update flag variable name

[dschuff]

lib/Target/WebAssembly/WebAssemblyFastISel.cpp

690 ↗

(On Diff #159796)

I don't think it's necessary to have checks at every single place where these types are used. It's enough e.g. to prevent them from reaching here with a check somewhere else, but still handle everything once the instructions get here. (Basically the same reason we have unconditional definitions and handling for simd instructions in some places, but have checks on the SIMD cpu feature in other places.

[tlively]

• Remove all redundant checks

[tlively]

Without all the checks I believe to be redundant this CL is much, much smaller. I'm concerned that it's possible the checks could save me a great deal of pain down the road, though.

[tlively]

@aheejin I think this is ready to land if it looks good to you

lib/Target/WebAssembly/WebAssemblyISelLowering.cpp

 #include "llvm/Support/Debug.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Target/TargetOptions.h"
 using namespace llvm;
 
 #define DEBUG_TYPE "wasm-lower"
 
+// Emit proposed instructions that may not have been implemented in engines
+cl::opt<bool> EnableUnimplementedWasmSIMDInstrs(
+    "wasm-enable-unimplemented-simd",
+    cl::desc("Emit potentially-unimplemented WebAssembly SIMD instructions"),
+    cl::init(false));
+
 WebAssemblyTargetLowering::WebAssemblyTargetLowering(
     const TargetMachine &TM, const WebAssemblySubtarget &STI)
     : TargetLowering(TM), Subtarget(&STI) {
   auto MVTPtr = Subtarget->hasAddr64() ? MVT::i64 : MVT::i32;
 
   // Booleans always contain 0 or 1.
   setBooleanContents(ZeroOrOneBooleanContent);
   // WebAssembly does not produce floating-point exceptions on normal floating
   // point operations.
   setHasFloatingPointExceptions(false);
   // We don't know the microarchitecture here, so just reduce register pressure.
   setSchedulingPreference(Sched::RegPressure);
   // Tell ISel that we have a stack pointer.
   setStackPointerRegisterToSaveRestore(
       Subtarget->hasAddr64() ? WebAssembly::SP64 : WebAssembly::SP32);
   // Set up the register classes.
   addRegisterClass(MVT::i32, &WebAssembly::I32RegClass);
   addRegisterClass(MVT::i64, &WebAssembly::I64RegClass);
   addRegisterClass(MVT::f32, &WebAssembly::F32RegClass);
   addRegisterClass(MVT::f64, &WebAssembly::F64RegClass);
   if (Subtarget->hasSIMD128()) {
     addRegisterClass(MVT::v16i8, &WebAssembly::V128RegClass);
     addRegisterClass(MVT::v8i16, &WebAssembly::V128RegClass);
     addRegisterClass(MVT::v4i32, &WebAssembly::V128RegClass);
-    addRegisterClass(MVT::v2i64, &WebAssembly::V128RegClass);
     addRegisterClass(MVT::v4f32, &WebAssembly::V128RegClass);
+    if (EnableUnimplementedWasmSIMDInstrs) {
+      addRegisterClass(MVT::v2i64, &WebAssembly::V128RegClass);
       addRegisterClass(MVT::v2f64, &WebAssembly::V128RegClass);
     }
+  }
   // Compute derived properties from the register classes.
   computeRegisterProperties(Subtarget->getRegisterInfo());
 
   setOperationAction(ISD::GlobalAddress, MVTPtr, Custom);
   setOperationAction(ISD::ExternalSymbol, MVTPtr, Custom);
   setOperationAction(ISD::JumpTable, MVTPtr, Custom);
   setOperationAction(ISD::BlockAddress, MVTPtr, Custom);
   setOperationAction(ISD::BRIND, MVT::Other, Custom);

test/CodeGen/WebAssembly/simd-arith.ll

-; RUN: llc < %s -asm-verbose=false -disable-wasm-fallthrough-return-opt -disable-wasm-explicit-locals -mattr=+simd128 | FileCheck %s --check-prefixes CHECK,SIMD128
-; RUN: llc < %s -asm-verbose=false -disable-wasm-fallthrough-return-opt -disable-wasm-explicit-locals -mattr=+simd128 -fast-isel | FileCheck %s --check-prefixes CHECK,SIMD128
+; RUN: llc < %s -asm-verbose=false -disable-wasm-fallthrough-return-opt -disable-wasm-explicit-locals -wasm-enable-unimplemented-simd -mattr=+simd128 | FileCheck %s --check-prefixes CHECK,SIMD128
+; RUN: llc < %s -asm-verbose=false -disable-wasm-fallthrough-return-opt -disable-wasm-explicit-locals -wasm-enable-unimplemented-simd -mattr=+simd128 -fast-isel | FileCheck %s --check-prefixes CHECK,SIMD128
+; RUN: llc < %s -asm-verbose=false -disable-wasm-fallthrough-return-opt -disable-wasm-explicit-locals -mattr=+simd128 | FileCheck %s --check-prefixes CHECK,SIMD128-VM
+; RUN: llc < %s -asm-verbose=false -disable-wasm-fallthrough-return-opt -disable-wasm-explicit-locals -mattr=+simd128 -fast-isel | FileCheck %s --check-prefixes CHECK,SIMD128-VM
 ; RUN: llc < %s -asm-verbose=false -disable-wasm-fallthrough-return-opt -disable-wasm-explicit-locals -mattr=-simd128 | FileCheck %s --check-prefixes CHECK,NO-SIMD128
 ; RUN: llc < %s -asm-verbose=false -disable-wasm-fallthrough-return-opt -disable-wasm-explicit-locals -mattr=-simd128 -fast-isel | FileCheck %s --check-prefixes CHECK,NO-SIMD128
 
 ; Test that basic SIMD128 arithmetic operations assemble as expected.
 
 target datalayout = "e-m:e-p:32:32-i64:64-n32:64-S128"
 target triple = "wasm32-unknown-unknown"
 
 ; ==============================================================================
 ; 16 x i8
 ; ==============================================================================
 ; CHECK-LABEL: add_v16i8
 ; NO-SIMD128-NOT: i8x16
 ; SIMD128: .param v128, v128{{$}}
 ; SIMD128: .result v128{{$}}
define <4 x i32> @mul_v4i32(<4 x i32> %x, <4 x i32> %y) {
   ret <4 x i32> %a
 }
 
 ; ==============================================================================
 ; 2 x i64
 ; ==============================================================================
 ; CHECK-LABEL: add_v2i64
 ; NO-SIMD128-NOT: i64x2
+; SIMD128-VM-NOT: i64x2
 ; SIMD128: .param v128, v128{{$}}
 ; SIMD128: .result v128{{$}}
 ; SIMD128: i64x2.add $push0=, $0, $1{{$}}
 ; SIMD128: return $pop0{{$}}
 define <2 x i64> @add_v2i64(<2 x i64> %x, <2 x i64> %y) {
   %a = add <2 x i64> %x, %y
   ret <2 x i64> %a
 }
 
 ; CHECK-LABEL: sub_v2i64
 ; NO-SIMD128-NOT: i64x2
+; SIMD128-VM-NOT: i64x2
 ; SIMD128: .param v128, v128{{$}}
 ; SIMD128: .result v128{{$}}
 ; SIMD128: i64x2.sub $push0=, $0, $1{{$}}
 ; SIMD128: return $pop0{{$}}
 define <2 x i64> @sub_v2i64(<2 x i64> %x, <2 x i64> %y) {
   %a = sub <2 x i64> %x, %y
   ret <2 x i64> %a
 }
 
 ; CHECK-LABEL: mul_v2i64
 ; NO-SIMD128-NOT: i64x2
+; SIMD128-VM-NOT: i64x2
 ; SIMD128: .param v128, v128{{$}}
 ; SIMD128: .result v128{{$}}
 ; SIMD128: i64x2.mul $push0=, $0, $1{{$}}
 ; SIMD128: return $pop0{{$}}
 define <2 x i64> @mul_v2i64(<2 x i64> %x, <2 x i64> %y) {
   %a = mul <2 x i64> %x, %y
   ret <2 x i64> %a
 }
define <4 x float> @mul_v4f32(<4 x float> %x, <4 x float> %y) {
   ret <4 x float> %a
 }
 
 ; ==============================================================================
 ; 2 x double
 ; ==============================================================================
 ; CHECK-LABEL: add_v2f64
 ; NO-SIMD128-NOT: f64x2
+; SIMD129-VM-NOT: f62x2
 ; SIMD128: .param v128, v128{{$}}
 ; SIMD128: .result v128{{$}}
 ; SIMD128: f64x2.add $push0=, $0, $1{{$}}
 ; SIMD128: return $pop0{{$}}
 define <2 x double> @add_v2f64(<2 x double> %x, <2 x double> %y) {
   %a = fadd <2 x double> %x, %y
   ret <2 x double> %a
 }
 
 ; CHECK-LABEL: sub_v2f64
 ; NO-SIMD128-NOT: f64x2
+; SIMD129-VM-NOT: f62x2
 ; SIMD128: .param v128, v128{{$}}
 ; SIMD128: .result v128{{$}}
 ; SIMD128: f64x2.sub $push0=, $0, $1{{$}}
 ; SIMD128: return $pop0{{$}}
 define <2 x double> @sub_v2f64(<2 x double> %x, <2 x double> %y) {
   %a = fsub <2 x double> %x, %y
   ret <2 x double> %a
 }
 
 ; CHECK-LABEL: div_v2f64
 ; NO-SIMD128-NOT: f64x2
+; SIMD129-VM-NOT: f62x2
 ; SIMD128: .param v128, v128{{$}}
 ; SIMD128: .result v128{{$}}
 ; SIMD128: f64x2.div $push0=, $0, $1{{$}}
 ; SIMD128: return $pop0{{$}}
 define <2 x double> @div_v2f64(<2 x double> %x, <2 x double> %y) {
   %a = fdiv <2 x double> %x, %y
   ret <2 x double> %a
 }
 
 ; CHECK-LABEL: mul_v2f64
 ; NO-SIMD128-NOT: f64x2
+; SIMD129-VM-NOT: f62x2
 ; SIMD128: .param v128, v128{{$}}
 ; SIMD128: .result v128{{$}}
 ; SIMD128: f64x2.mul $push0=, $0, $1{{$}}
 ; SIMD128: return $pop0{{$}}
 define <2 x double> @mul_v2f64(<2 x double> %x, <2 x double> %y) {
   %a = fmul <2 x double> %x, %y
   ret <2 x double> %a
 }