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: ```llvm 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](https://github.com/llvm/llvm-project/blob/fb5a38bb4930736b0aab3ce428b60245921f982f/llvm/lib/Target/WebAssembly/WebAssemblySubtarget.h#L107) 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, https://github.com/llvm/llvm-project/blob/fb5a38bb4930736b0aab3ce428b60245921f982f/llvm/lib/Target/WebAssembly/WebAssemblySubtarget.h#L107

The reason this fails is we query the feature set directly here: https://github.com/llvm/llvm-project/blob/d0d05aec3b6792136a9f75eb85dd2ea66005ae12/llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp#L644-L645

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. https://github.com/llvm/llvm-project/blob/d0d05aec3b6792136a9f75eb85dd2ea66005ae12/clang/lib/Driver/ToolChains/WebAssembly.cpp#L331-L369

Also this will fail in AsmPrinter for the same reason that https://github.com/llvm/llvm-project/issues/98502#issuecomment-2224034839 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: https://github.com/llvm/llvm-project/blob/0caf0c93e759816663af52e8632d1c3953dbc715/llvm/lib/Target/X86/X86MCInstLower.cpp#L2188-L2190 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: https://github.com/llvm/llvm-project/blob/0caf0c93e759816663af52e8632d1c3953dbc715/llvm/lib/Target/WebAssembly/WebAssemblyInstrInfo.td#L79-L81

but while X86 has many features (= Predicates), it only has five AssemblerPredicates: https://github.com/llvm/llvm-project/blob/0caf0c93e759816663af52e8632d1c3953dbc715/llvm/lib/Target/X86/X86InstrPredicates.td#L187-L198 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 https://github.com/llvm/llvm-project/pull/99803, it's much appreciated!