[PATCH] D56504: [WebAssembly] Add unimplemented-simd128 feature, gate builtins

[Heejin Ahn via Phabricator via cfe-commits]

aheejin added inline comments.


================
Comment at: include/clang/Basic/BuiltinsWebAssembly.def:53
 // Saturating fp-to-int conversions
-BUILTIN(__builtin_wasm_trunc_saturate_s_i32_f32, "if", "nc")
-BUILTIN(__builtin_wasm_trunc_saturate_u_i32_f32, "if", "nc")
-BUILTIN(__builtin_wasm_trunc_saturate_s_i32_f64, "id", "nc")
-BUILTIN(__builtin_wasm_trunc_saturate_u_i32_f64, "id", "nc")
-BUILTIN(__builtin_wasm_trunc_saturate_s_i64_f32, "LLif", "nc")
-BUILTIN(__builtin_wasm_trunc_saturate_u_i64_f32, "LLif", "nc")
-BUILTIN(__builtin_wasm_trunc_saturate_s_i64_f64, "LLid", "nc")
-BUILTIN(__builtin_wasm_trunc_saturate_u_i64_f64, "LLid", "nc")
-
-// Floating point min/max
-BUILTIN(__builtin_wasm_min_f32, "fff", "nc")
-BUILTIN(__builtin_wasm_max_f32, "fff", "nc")
-BUILTIN(__builtin_wasm_min_f64, "ddd", "nc")
-BUILTIN(__builtin_wasm_max_f64, "ddd", "nc")
+TARGET_BUILTIN(__builtin_wasm_trunc_saturate_s_i32_f32, "if", "nc", "nontrapping-fptoint")
+TARGET_BUILTIN(__builtin_wasm_trunc_saturate_u_i32_f32, "if", "nc", "nontrapping-fptoint")
----------------
clang-format this file


================
Comment at: lib/Basic/Targets/WebAssembly.cpp:89
+    // features control availability of builtins
+    setSIMDLevel(Features, SIMDLevel);
+    if (HasNontrappingFPToInt)
----------------
Minor thing, but should we extract this as a function? It is gonna be a couple line anyway, like

```
if (CPU == "bleeding-edge") {
  ...
  Features["unimplemented-simd128"] = Features["simd128"] = true;
}

if (SIMDLevel >= SIMD128)
  Features["simd128"] = true;
if (SIMDLevel >= UnimplementedSIMD128)
  Features["unimplemented-simd128"] = true;
...

And to me it is more readable to see all `Features` setting in one place. But I'm not too opinionated either.


================
Comment at: lib/Basic/Targets/WebAssembly.cpp:98
+    return TargetInfo::initFeatureMap(Features, Diags, CPU, FeaturesVec);
+  }
+
----------------
The indentation of these functions looks weird and there are lines that exceeds 80 cols. clang-format?


================
Comment at: test/CodeGen/builtins-wasm.c:2
+// RUN: %clang_cc1 -triple wasm32-unknown-unknown -target-feature +unimplemented-simd128 -target-feature +nontrapping-fptoint -target-feature +exception-handling -fno-lax-vector-conversions -O3 -emit-llvm -o - %s | FileCheck %s -check-prefixes WEBASSEMBLY,WEBASSEMBLY32
+// RUN: %clang_cc1 -triple wasm64-unknown-unknown -target-feature +unimplemented-simd128 -target-feature +nontrapping-fptoint -target-feature +exception-handling -fno-lax-vector-conversions -O3 -emit-llvm -o - %s | FileCheck %s -check-prefixes WEBASSEMBLY,WEBASSEMBLY64
 
----------------
Maybe we can add a line that disables one of the target features and make sure it fails? You can do something like
```
RUN: not %clang_cc1 ...
```
to see if it fails.


Repository:
  rC Clang

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

https://reviews.llvm.org/D56504