This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Add unimplemented-simd128 feature, gate builtins
ClosedPublic

Authored by tlively on Jan 9 2019, 10:29 AM.

Details

Reviewers
aheejin
dschuff
Summary

Depends on D56501. Also adds a macro define
__wasm_unimplemented_simd128__ for feature detection of unimplemented
SIMD builtins.

Diff Detail

Event Timeline

tlively created this revision.Jan 9 2019, 10:29 AM
tlively updated this revision to Diff 180921.Jan 9 2019, 1:40 PM
  • Decouple sign-ext from simd128
aheejin added inline comments.Jan 9 2019, 2:46 PM
include/clang/Basic/BuiltinsWebAssembly.def
53–60

clang-format this file

lib/Basic/Targets/WebAssembly.cpp
125

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.
134

The indentation of these functions looks weird and there are lines that exceeds 80 cols. clang-format?

test/CodeGen/builtins-wasm.c
2

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.

tlively updated this revision to Diff 180984.Jan 9 2019, 7:04 PM
  • Match the new naming scheme from rL350791
tlively retitled this revision from [WebAssembly] Add unimplemented-simd128 feature, gate builtins to [WebAssembly] Add simd128-unimplemented feature, gate builtins.Jan 9 2019, 7:19 PM
tlively edited the summary of this revision. (Show Details)
tlively updated this revision to Diff 180989.Jan 9 2019, 7:23 PM
tlively marked 3 inline comments as done.
  • Fix formatting, fix and test macro name
tlively added inline comments.Jan 9 2019, 7:23 PM
lib/Basic/Targets/WebAssembly.cpp
125

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.

134

Done, and copied my pre-commit git hooks from the main LLVM repo so it won't be an issue again.

aheejin added inline comments.Jan 9 2019, 9:57 PM
include/clang/Basic/BuiltinsWebAssembly.def
53–60

This file still does not look like clang-formatted; I guess your script misses this file because its extension is def.

tlively retitled this revision from [WebAssembly] Add simd128-unimplemented feature, gate builtins to [WebAssembly] Add unimplemented-simd128 feature, gate builtins.Jan 10 2019, 3:10 PM
tlively edited the summary of this revision. (Show Details)
tlively updated this revision to Diff 181171.Jan 10 2019, 3:14 PM
tlively marked an inline comment as done.
  • Change names again to reflect latest committed LLVM change
tlively added inline comments.Jan 10 2019, 3:15 PM
include/clang/Basic/BuiltinsWebAssembly.def
53–60

As discussed, clang-format might be bad for readability in this file.

aheejin accepted this revision.Jan 10 2019, 3:49 PM
This revision is now accepted and ready to land.Jan 10 2019, 3:49 PM
tlively closed this revision.Jan 10 2019, 4:27 PM

Closed by commit rC350909.