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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
