[PATCH] D77908: [WebAssembly] Enable nontrapping-fptoint for `default` cpu

Thomas Lively via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Fri Apr 10 15:04:18 PDT 2020


tlively added a comment.

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



================
Comment at: clang/lib/Basic/Targets/WebAssembly.h:33
 
-  bool HasNontrappingFPToInt = false;
+  bool HasNontrappingFPToInt = true;
   bool HasSignExt = false;
----------------
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.


================
Comment at: clang/test/Preprocessor/wasm-target-features.c:10
+
+// NONTRAPPING-FPTOINT:#define __wasm_nontrapping_fptoint__ 1{{$}}
+
----------------
Is there a RUN line corresponding to this check anymore?


================
Comment at: lld/test/wasm/data-layout.ll:72
 ; RUN: wasm-ld -no-gc-sections --allow-undefined --no-entry --shared-memory \
-; RUN:     --features=atomics,bulk-memory --initial-memory=131072 \
+; RUN:     --features=atomics,bulk-memory,nontrapping-fptoint --initial-memory=131072 \
 ; RUN:     --max-memory=131072 -o %t_max.wasm %t.o %t.hello.o
----------------
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.


================
Comment at: lld/test/wasm/import-memory.test:36
+# RUN: wasm-ld --import-memory --shared-memory \
+# RUN:     --features=nontrapping-fptoint,atomics,bulk-memory \
 # RUN:     --initial-memory=262144 --max-memory=327680 -o %t.max.wasm %t.start.o
----------------
Same here about using the mvp cpu.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp:229
+      if (!Features[KV.Value] && Defaults[KV.Value])
+        Ret += (StringRef("-") + KV.Key + ",").str();
     }
----------------
Nice!


================
Comment at: llvm/test/CodeGen/WebAssembly/multivalue.ll:259
+; CHECK-NEXT: .ascii "nontrapping-fptoint"
+; CHECK-NEXT: .int8 43
 ; CHECK-NEXT: .int8 9
----------------
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.


================
Comment at: llvm/test/MC/WebAssembly/array-fill.ll:27
 ; CHECK-NEXT:         Flags:           [ ]
-; CHECK-NEXT: ...
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77908/new/

https://reviews.llvm.org/D77908





More information about the cfe-commits mailing list