WebAssembly: simd128 feature is required even when the relaxed-simd feature is enabled

[alexcrichton]

Given this input:

target triple = "wasm32-unknown-wasi"

declare <2 x i64> @llvm.wasm.relaxed.laneselect.v2i64(<2 x i64>, <2 x i64>, <2 x i64>)

define <2 x i64> @test(<2 x i64>, <2 x i64>, <2 x i64>) #0 {
start:
  %_4 = tail call <2 x i64> @llvm.wasm.relaxed.laneselect.v2i64(<2 x i64> %0, <2 x i64> %1, <2 x i64> %2) #3
  ret <2 x i64> %_4
}

attributes #0 = { "target-features"="+relaxed-simd" }

this currently crashes with:

$ llc foo.ll -filetype=obj -o foo.o
LLVM ERROR: Attempting to emit LOCAL_GET_V128 instruction but the Feature_HasSIMD128 predicate(s) are not met

This works if +simd128 is added but given this it seems like this might be intended to work as-is instead of requiring an explicit enabling of the +simd128 feature.

[alexcrichton]

cc @tlively

[llvmbot]

@llvm/issue-subscribers-backend-webassembly

Author: Alex Crichton (alexcrichton)

Given this input:

target triple = "wasm32-unknown-wasi"

declare &lt;2 x i64&gt; @<!-- -->llvm.wasm.relaxed.laneselect.v2i64(&lt;2 x i64&gt;, &lt;2 x i64&gt;, &lt;2 x i64&gt;)

define &lt;2 x i64&gt; @<!-- -->test(&lt;2 x i64&gt;, &lt;2 x i64&gt;, &lt;2 x i64&gt;) #<!-- -->0 {
start:
  %_4 = tail call &lt;2 x i64&gt; @<!-- -->llvm.wasm.relaxed.laneselect.v2i64(&lt;2 x i64&gt; %0, &lt;2 x i64&gt; %1, &lt;2 x i64&gt; %2) #<!-- -->3
  ret &lt;2 x i64&gt; %_4
}

attributes #<!-- -->0 = { "target-features"="+relaxed-simd" }

this currently crashes with:

$ llc foo.ll -filetype=obj -o foo.o
LLVM ERROR: Attempting to emit LOCAL_GET_V128 instruction but the Feature_HasSIMD128 predicate(s) are not met

This works if +simd128 is added but given this it seems like this might be intended to work as-is instead of requiring an explicit enabling of the +simd128 feature.

[dschuff]

You're saying that maybe enabling the relaxed-simd feature should implicitly enable the simd128 feature, rather than requiring it to be explicit?
Even if not, it probably makes sense to at least have an explicit error message about the feature rather than having a more obscure failure down the line during isel or whatever.

[alexcrichton]

Ideally yeah I think that'd be a good state. I thought that was the intention already but it may not be. I believe in x86 if you enable sse4.2 for example it auto-enables sse4.1 and prior, but I'm not sure by what mechanism that happens. Or at least that's what I'm led to believe which is where my pseudo-expectation for enabling simd128 automatically comes from.

If this goes the other way and it's an intentional error then frontends will need to be updated so that when users enable relaxed-simd then the frontend needs to also present a first-class error or auto-enable simd128. For example rustc doesn't do any sort of auto-enabling of dependent features right now (even for other platforms, as far as I know), so this'd be a new feature to implement there.

[dschuff]

@aheejin we must do this for exceptions too, right (i.e. implicitly enable reference types when enabling exnref). I'm guessing that happens in the frontend though, since the frontend is already coordinating several different flags.

[tlively]

I know the clang frontend driver has a “feature tower” so simd128 will be automatically enabled if you use relaxed-simd, but I guess we don’t do that in the backend. The intention is to follow whatever pattern other backends use.

[aheejin]

@alexcrichton Even though we check this as you pointed out,

llvm-project/llvm/lib/Target/WebAssembly/WebAssemblySubtarget.h

Line 107 in fb5a38b

bool hasSIMD128() const { return SIMDLevel >= SIMD128; }

The reason this fails is we query the feature set directly here:

llvm-project/llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp

Lines 644 to 645 in d0d05ae

	WebAssembly_MC::verifyInstructionPredicates(MI->getOpcode(),
	Subtarget->getFeatureBits());

This verifyInstructionPredicates function (and other functions called by this function) is generated by https://github.com/llvm/llvm-project/blob/main/llvm/utils/TableGen/InstrInfoEmitter.cpp, and looks like this (you can check it in the lib/Target/WebAssembly/WebAssemblyGenInstrInfo.inc in your build directory):

void verifyInstructionPredicates(                                                
    unsigned Opcode, const FeatureBitset &Features) {
#ifndef NDEBUG
  FeatureBitset AvailableFeatures = computeAvailableFeatures(Features);
  FeatureBitset RequiredFeatures = computeRequiredFeatures(Opcode);    
  FeatureBitset MissingFeatures =
      (AvailableFeatures & RequiredFeatures) ^
      RequiredFeatures;
  if (MissingFeatures.any()) {
    std::ostringstream Msg;
    Msg << "Attempting to emit " << &WebAssemblyInstrNameData[WebAssemblyInstrNameIndices[Opcode]]
        << " instruction but the ";
    for (unsigned i = 0, e = MissingFeatures.size(); i != e; ++i)
      if (MissingFeatures.test(i))
        Msg << SubtargetFeatureNames[i] << " ";
    Msg << "predicate(s) are not met";
    report_fatal_error(Msg.str().c_str());
  }
#endif // NDEBUG
}

And computeAvailableFeatures there is just a set query, like this:

inline FeatureBitset computeAvailableFeatures(const FeatureBitset &FB) {         
  FeatureBitset Features;
  if (FB[WebAssembly::FeatureAtomics])
    Features.set(Feature_HasAtomicsBit);
  if (FB[WebAssembly::FeatureBulkMemory])
    Features.set(Feature_HasBulkMemoryBit);
  if (FB[WebAssembly::FeatureExceptionHandling])                           
    Features.set(Feature_HasExceptionHandlingBit);
  ...

And it looks other targets' AsmPrinter checks the feature this way too: https://github.com/search?q=repo%3Allvm%2Fllvm-project%20verifyInstructionPredicates&type=code

So yeah as @tlively said we seem to follow the pattern that other targets are using. Not sure whether there is another mechanical way to do this kind of checks within AsmPrinter.

[aheejin]

@dschuff

@aheejin we must do this for exceptions too, right (i.e. implicitly enable reference types when enabling exnref). I'm guessing that happens in the frontend though, since the frontend is already coordinating several different flags.

We can probably check things like this in the backend as well, but not sure if it is worth duplicating the effort, given that EH may not be only thing to be checked this way.

llvm-project/clang/lib/Driver/ToolChains/WebAssembly.cpp

Lines 331 to 369 in d0d05ae

	if (DriverArgs.getLastArg(options::OPT_fwasm_exceptions)) {
	// '-fwasm-exceptions' is not compatible with '-mno-exception-handling'
	if (DriverArgs.hasFlag(options::OPT_mno_exception_handing,
	options::OPT_mexception_handing, false))
	getDriver().Diag(diag::err_drv_argument_not_allowed_with)
	<< "-fwasm-exceptions"
	<< "-mno-exception-handling";
	// '-fwasm-exceptions' is not compatible with
	// '-mllvm -enable-emscripten-cxx-exceptions'
	for (const Arg *A : DriverArgs.filtered(options::OPT_mllvm)) {
	if (StringRef(A->getValue(0)) == "-enable-emscripten-cxx-exceptions")
	getDriver().Diag(diag::err_drv_argument_not_allowed_with)
	<< "-fwasm-exceptions"
	<< "-mllvm -enable-emscripten-cxx-exceptions";
	}
	// '-fwasm-exceptions' implies exception-handling feature
	CC1Args.push_back("-target-feature");
	CC1Args.push_back("+exception-handling");
	// Backend needs -wasm-enable-eh to enable Wasm EH
	CC1Args.push_back("-mllvm");
	CC1Args.push_back("-wasm-enable-eh");
	// New Wasm EH spec (adopted in Oct 2023) requires multivalue and
	// reference-types.
	if (DriverArgs.hasFlag(options::OPT_mno_multivalue,
	options::OPT_mmultivalue, false)) {
	getDriver().Diag(diag::err_drv_argument_not_allowed_with)
	<< "-fwasm-exceptions" << "-mno-multivalue";
	}
	if (DriverArgs.hasFlag(options::OPT_mno_reference_types,
	options::OPT_mreference_types, false)) {
	getDriver().Diag(diag::err_drv_argument_not_allowed_with)
	<< "-fwasm-exceptions" << "-mno-reference-types";
	}
	CC1Args.push_back("-target-feature");
	CC1Args.push_back("+multivalue");
	CC1Args.push_back("-target-feature");
	CC1Args.push_back("+reference-types");
	}

Also this will fail in AsmPrinter for the same reason that #98502 (comment) describes.

[alexcrichton]

I'm not sure how it works, but the point of comparison for other backends for me is:

target triple = "x86_64-unknown-linux-gnu"

define <8 x i16> @test(<8 x i16> %a) #0 {
start:
  %b = tail call <8 x i16> @llvm.x86.sse41.phminposuw(<8 x i16> %a) 
  ret <8 x i16> %b
}

declare <8 x i16> @llvm.x86.sse41.phminposuw(<8 x i16>) 

attributes #0 = { "target-features"="+sse4.2" }

Here only SSE4.2 is enabled but an SSE4.1 intrinsic is used and the backend doesn't fail with this.

[aheejin]

X86 has that verification commented out:

llvm-project/llvm/lib/Target/X86/X86MCInstLower.cpp

Lines 2188 to 2190 in 0caf0c9

	// FIXME: Enable feature predicate checks once all the test pass.
	// X86_MC::verifyInstructionPredicates(MI->getOpcode(),
	// Subtarget->getFeatureBits());

But this is not the reason your SSE test works. Even if I revive this line your test still works fine.

The reason is what AsmPrinter checks in verifyInstructionPredicates is not Predicates but AssemblerPredicates. We define each target feature as a Predicate and also an AssemblerPredicate, like this:

llvm-project/llvm/lib/Target/WebAssembly/WebAssemblyInstrInfo.td

Lines 79 to 81 in 0caf0c9

	def HasSIMD128 :
	Predicate<"Subtarget->hasSIMD128()">,
	AssemblerPredicate<(all_of FeatureSIMD128), "simd128">;

but while X86 has many features (= Predicates), it only has five AssemblerPredicates:

llvm-project/llvm/lib/Target/X86/X86InstrPredicates.td

Lines 187 to 198 in 0caf0c9

	def Not64BitMode : Predicate<"!Subtarget->is64Bit()">,
	AssemblerPredicate<(all_of (not Is64Bit)), "Not 64-bit mode">;
	def In64BitMode : Predicate<"Subtarget->is64Bit()">,
	AssemblerPredicate<(all_of Is64Bit), "64-bit mode">;
	def IsLP64 : Predicate<"Subtarget->isTarget64BitLP64()">;
	def NotLP64 : Predicate<"!Subtarget->isTarget64BitLP64()">;
	def In16BitMode : Predicate<"Subtarget->is16Bit()">,
	AssemblerPredicate<(all_of Is16Bit), "16-bit mode">;
	def Not16BitMode : Predicate<"!Subtarget->is16Bit()">,
	AssemblerPredicate<(all_of (not Is16Bit)), "Not 16-bit mode">;
	def In32BitMode : Predicate<"Subtarget->is32Bit()">,
	AssemblerPredicate<(all_of Is32Bit), "32-bit mode">;

So its computeAvailableFeature in X86GenInstrInfo.inc is very simple and it doesn't check things like SSE:

inline FeatureBitset computeAvailableFeatures(const FeatureBitset &FB) {
  FeatureBitset Features;
  if (!FB[X86::Is64Bit])
    Features.set(Feature_Not64BitModeBit);
  if (FB[X86::Is64Bit])
    Features.set(Feature_In64BitModeBit);
  if (FB[X86::Is16Bit])
    Features.set(Feature_In16BitModeBit);
  if (!FB[X86::Is16Bit])
    Features.set(Feature_Not16BitModeBit);
  if (FB[X86::Is32Bit])
    Features.set(Feature_In32BitModeBit);
  return Features;
}

Anyway, for Wasm I don't think we necessarily need to make Predicates and AssemblerPredicates separate; there seems to be a simpler fix for that. I uploaded it: #99803

[alexcrichton]

Aha makes sense, thanks for digging through that! Additionally thanks for #99803, it's much appreciated!