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

[Thomas Lively via Phabricator via cfe-commits]

tlively added inline comments.


================
Comment at: lib/Basic/Targets/WebAssembly.cpp:89
+    // features control availability of builtins
+    setSIMDLevel(Features, SIMDLevel);
+    if (HasNontrappingFPToInt)
----------------
aheejin wrote:
> 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.
The structure of basically all this code is pulled from X86.cpp, which is obviously has a lot more features to wrangle. This particular function is similar to `setSSELevel` in X86.cpp. I agree that it probably doesn't need to be separate now, but as we explore possible extensions to the SIMD proposal in the future I think it will be useful to have this function.


================
Comment at: lib/Basic/Targets/WebAssembly.cpp:98
+    return TargetInfo::initFeatureMap(Features, Diags, CPU, FeaturesVec);
+  }
+
----------------
aheejin wrote:
> The indentation of these functions looks weird and there are lines that exceeds 80 cols. clang-format?
Done, and copied my pre-commit git hooks from the main LLVM repo so it won't be an issue again.


Repository:
  rC Clang

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

https://reviews.llvm.org/D56504