Page MenuHomePhabricator

[WebAssembly] Enable nontrapping-fptoint for `default` cpu
Needs ReviewPublic

Authored by sbc100 on Apr 10 2020, 2:08 PM.

Details

Reviewers
tlively
sunfish
Summary

Since this feature is now merged into the upstream wasm spec it
makes sense to enable it by default, at least for the default CPU.
The mvp CPU can still be used to avoid this feature.

See: https://github.com/WebAssembly/spec/pull/1143

This is just to test the water and the plan is to enable all features
that have been merged, assuming this change is accepted.

Diff Detail

Unit TestsFailed

TimeTest
40 msClang.CodeGen::Unknown Unit Message ("")
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/workspace/BETA_amd64_debian_testing_clang8/llvm-project/build/bin/clang -cc1 -internal-isystem /mnt/disks/ssd0/agent/workspace/BETA_amd64_debian_testing_clang8/llvm-project/build/lib/clang/11.0.0/include -nostdsysteminc -triple wasm32-unknown-unknown -target-feature +unimplemented-simd128 -target-feature +exception-handling -target-feature +bulk-memory -flax-vector-conversions=none -O3 -emit-llvm -o - /mnt/disks/ssd0/agent/workspace/BETA_amd64_debian_testing_clang8/llvm-project/clang/test/CodeGen/builtins-wasm.c | /mnt/disks/ssd0/agent/workspace/BETA_amd64_debian_testing_clang8/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/workspace/BETA_amd64_debian_testing_clang8/llvm-project/clang/test/CodeGen/builtins-wasm.c -check-prefixes WEBASSEMBLY,WEBASSEMBLY32
100 msClang.CodeGen::Unknown Unit Message ("")
Script: -- : 'RUN: at line 1'; c:\ws\workspace\amd64_windows_vs2017\llvm-project\build\bin\clang.exe -cc1 -internal-isystem c:\ws\workspace\amd64_windows_vs2017\llvm-project\build\lib\clang\11.0.0\include -nostdsysteminc -triple wasm32-unknown-unknown -target-feature +unimplemented-simd128 -target-feature +exception-handling -target-feature +bulk-memory -flax-vector-conversions=none -O3 -emit-llvm -o - C:\ws\workspace\amd64_windows_vs2017\llvm-project\clang\test\CodeGen\builtins-wasm.c | c:\ws\workspace\amd64_windows_vs2017\llvm-project\build\bin\filecheck.exe C:\ws\workspace\amd64_windows_vs2017\llvm-project\clang\test\CodeGen\builtins-wasm.c -check-prefixes WEBASSEMBLY,WEBASSEMBLY32
70 mslldb-shell.SymbolFile/DWARF::Unknown Unit Message ("")
Script: -- : 'RUN: at line 8'; /mnt/disks/ssd0/agent/workspace/BETA_amd64_debian_testing_clang8/llvm-project/build/bin/llvm-mc -triple=x86_64-pc-linux -filetype=obj /mnt/disks/ssd0/agent/workspace/BETA_amd64_debian_testing_clang8/llvm-project/lldb/test/Shell/SymbolFile/DWARF/debug_loc.s --defsym LOC=0 > /mnt/disks/ssd0/agent/workspace/BETA_amd64_debian_testing_clang8/llvm-project/build/tools/lldb/test/SymbolFile/DWARF/Output/debug_loc.s.tmp
140 mslldb-unit.Host/_/HostTests::Unknown Unit Message ("")
Note: Google Test filter = ConnectionFileDescriptorTest.TCPGetURIv6 [==========] Running 1 test from 1 test case. [----------] Global test environment set-up.

Event Timeline

sbc100 created this revision.Apr 10 2020, 2:08 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 10 2020, 2:08 PM

Do we have tests that check that passing -nontrapping-fptoint to llc (or clang) successfully disables the feature?

clang/lib/Basic/Targets/WebAssembly.h
33 ↗(On Diff #256662)

It seems strange to change the default here. x86 initializes all its corresponding features to false then selectively enables them in initFeatureMap. I think we should stick with that pattern if possible.

clang/test/Preprocessor/wasm-target-features.c
11

Is there a RUN line corresponding to this check anymore?

lld/test/wasm/data-layout.ll
72

Would it make more sense to use the MVP CPU to avoid the unnecessary nontrapping-fptoint? I feel like it just distracts from the point of the test here.

lld/test/wasm/import-memory.test
36

Same here about using the mvp cpu.

llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp
229

Nice!

llvm/test/CodeGen/WebAssembly/multivalue.ll
259

Might want to use mvp cpu here too, not because this is necessarily distracting, but because I don't see any value in updating this test every time we standardize another feature. Same with many other tests.

llvm/test/MC/WebAssembly/array-fill.ll
27

In some tests you just remove this line and in others you explicitly check for the start of the target features section. Would it make sense to just choose one or the other? FWIW I think checking for the start of the target features section makes the most sense.

sbc100 updated this revision to Diff 256712.Apr 10 2020, 5:17 PM
sbc100 marked 7 inline comments as done.

feedback

clang/lib/Basic/Targets/WebAssembly.h
33 ↗(On Diff #256662)

Right.. makes sense. Done.

clang/test/Preprocessor/wasm-target-features.c
11

Oops, deleted that line.

35

Here is t he test for disabling in clang.

For llc there was already a test for -mattr=-nontrapping-fptoint in the form of llvm/test/CodeGen/WebAssembly/conv-trap.ll. This is what caused me to need to fix getFeatureString.

sbc100 updated this revision to Diff 256713.Apr 10 2020, 5:18 PM

err.. arc diff did something funny.

Looks good! I don't have anything to add beyond Thomas' review comments.

Is the general plan for LLVM documented somewhere?

It's not obvious to me why something being in the wasm spec means it should be enabled by default in LLVM. (It's also not obvious to me that is wrong! I'm just not sure what the reasoning is here.)

In particular, something being in the wasm spec doesn't mean it is widespread yet. I see https://github.com/emscripten-core/emscripten/pull/10885 proposes to diverge the emscripten defaults from LLVMs. That seems odd to me - why shouldn't those be the same? If they aren't the same it's a potential source of confusion in multiple ways. For example, if the reasoning is "LLVM moves with the spec, emscripten moves with the web" then that means *non*-emscripten toolchains targeting the web have more work to do. Also comparisons between toolchains will get harder to get apples-to-apples.

Again, not opposed to this, just not sure what the big picture is.

I don't have documented plan anywhere. I simply observed that we have CPUs called mvp and generic and it made sense to start including more things in the generic CPU, leaving the mvp option open to those who want to be conservative.

I don't think its a huge issue that emscripten should have different default cpu to llvm itself. Emscripten already passes flags to llvm and has different defaults. It even has its own target triple.

I confess my main goal here was to be able to encode a configuration which represents the "current spec", but its not "bleeding edge". This also happens to the CPU that the wasi SDK would want to use as I believe almost all the users of wasi-sdk are targeting VMs which include the current spec features.

As a less controversial version of this change I could instead create a new CPU called current and leave generic as is (basically leave it at mvp) until we can agree that a features is widespread enough to warrant being part of generic?

Having said that, any*non*-emscripten toolchains that are targeting the web already, like emscripten, have to decide which feature flags to pass by default. Its not something they can avoid. They need to have some opinion. I don't think that llvm and emscripten having different opinions on the default set of features to target is terrible outcome.

As a less controversial version of this change I could instead create a new CPU called current and leave generic as is (basically leave it at mvp) until we can agree that a features is widespread enough to warrant being part of generic?

That makes a lot of sense to me. current or current-spec or such seems pretty clear. Then generic stays as it always was (at mvp) and that seems safer for the ecosystem.

Btw, how does LLVM handle this issue with other backends? When say Intel releases a new CPU with a new feature, are those automatically applied in the generic CPU for Intel? (and hence people that want older CPUs will get breakage unless they change the CPU target) If those are automatically applied, at what frequency/policy?