[WebAssembly] Add gc target feature to addBleedingEdgeFeatures#151107
Merged
Conversation
Member
|
@llvm/pr-subscribers-clang-driver @llvm/pr-subscribers-clang Author: Hood Chatham (hoodmane) ChangesSee suggestion here: Full diff: https://github.com/llvm/llvm-project/pull/151107.diff 1 Files Affected:
diff --git a/clang/lib/Basic/Targets/WebAssembly.cpp b/clang/lib/Basic/Targets/WebAssembly.cpp
index e362350ea678f..2ea505874e38b 100644
--- a/clang/lib/Basic/Targets/WebAssembly.cpp
+++ b/clang/lib/Basic/Targets/WebAssembly.cpp
@@ -197,6 +197,7 @@ bool WebAssemblyTargetInfo::initFeatureMap(
Features["multimemory"] = true;
Features["tail-call"] = true;
Features["wide-arithmetic"] = true;
+ Features["gc"] = true;
setSIMDLevel(Features, RelaxedSIMD, true);
};
if (CPU == "generic") {
|
Member
|
@llvm/pr-subscribers-backend-webassembly Author: Hood Chatham (hoodmane) ChangesSee suggestion here: Full diff: https://github.com/llvm/llvm-project/pull/151107.diff 1 Files Affected:
diff --git a/clang/lib/Basic/Targets/WebAssembly.cpp b/clang/lib/Basic/Targets/WebAssembly.cpp
index e362350ea678f..2ea505874e38b 100644
--- a/clang/lib/Basic/Targets/WebAssembly.cpp
+++ b/clang/lib/Basic/Targets/WebAssembly.cpp
@@ -197,6 +197,7 @@ bool WebAssemblyTargetInfo::initFeatureMap(
Features["multimemory"] = true;
Features["tail-call"] = true;
Features["wide-arithmetic"] = true;
+ Features["gc"] = true;
setSIMDLevel(Features, RelaxedSIMD, true);
};
if (CPU == "generic") {
|
sbc100
reviewed
Jul 29, 2025
| Features["multimemory"] = true; | ||
| Features["tail-call"] = true; | ||
| Features["wide-arithmetic"] = true; | ||
| Features["gc"] = true; |
Contributor
There was a problem hiding this comment.
I would expect to see a corresponding test change, is there not test that needs updating?
Contributor
Author
There was a problem hiding this comment.
Well... the CI didn't fail... so maybe not? This might be completely redundant with:
https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/WebAssembly/WebAssembly.td?plain=1#L139-L146
so it suffices to add the feature in either of these two places.
Contributor
There was a problem hiding this comment.
If thats really the case we should remove the duplication.
Contributor
|
clang/test/Preprocessor/wasm-target-features.c looks like its includes references to bleeding edge.. not sure why it doesn't require an update. |
Contributor
I guess the test doesn't break when new features are added.. only if they are removed. Still, it should probably be updated. |
Member
|
You need to update https://github.com/llvm/llvm-project/blob/main/clang/test/Preprocessor/wasm-target-features.c and https://github.com/llvm/llvm-project/blob/main/clang/test/Driver/wasm-features.c Can you also alphabetize the list? So I think it'd be good to alphabetize other places too: (The list was sorted alphabetically before) https://github.com/llvm/llvm- project/blob/d2361e43d12d51b744a4131be7fab2aa3a79feab/clang/lib/Basic/Targets/WebAssembly.cpp#L110-L111 llvm-project/clang/lib/Basic/Targets/WebAssembly.cpp Lines 313 to 320 in d2361e4 llvm-project/llvm/lib/Target/WebAssembly/WebAssemblyInstrInfo.td Lines 79 to 80 in d2361e4 |
Contributor
Author
|
@aheejin okay I think I fixed it. |
Member
|
Thanks! One more nit: |
aheejin
reviewed
Jul 29, 2025
sbc100
reviewed
Jul 29, 2025
Member
|
https://github.com/llvm/llvm-project/blob/main/clang/test/Preprocessor/wasm-target-features.c has more places to update. For example, these are where llvm-project/clang/test/Preprocessor/wasm-target-features.c Lines 46 to 53 in 052b836 https://github.com/llvm/llvm-project/blob/main/clang/test/Driver/wasm-features.c needs to be updated too. For example, this is llvm-project/clang/test/Driver/wasm-features.c Lines 38 to 42 in 052b836 |
Contributor
Author
|
Okay I think I'm up to date with the comments so far. |
Member
It looks this is not updated? |
Contributor
Author
|
Thanks @aheejin. How's it look now? |
sbc100
approved these changes
Jul 29, 2025
Contributor
Author
|
Can someone merge it? |
Contributor
|
Looks like there are CI failures. Are they flakes? |
Member
|
Tests failures don't seem to be related to this PR. Merged. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/10/builds/10463 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/174/builds/21932 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/140/builds/27911 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/154/builds/19484 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/190/builds/24456 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/46/builds/20892 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/108/builds/15905 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/11/builds/20658 Here is the relevant piece of the build log for the reference |
Contributor
|
Hello! This PR breaks several bots. The error seems related to the changed files. Could you please fix it? Thanks.
|
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/95/builds/16418 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/23/builds/12626 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/55/builds/14907 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/168/builds/14713 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/208/builds/3210 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/24/builds/11011 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/94/builds/9325 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/164/builds/12047 Here is the relevant piece of the build log for the reference |
Contributor
|
This change is causing failures on quite a few bots, can we either fix or revert this soon? |
vitalybuka
pushed a commit
that referenced
this pull request
Jul 30, 2025
Contributor
|
@hoodmane The patch was reverted. |
llvm-sync Bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Jul 30, 2025
…EdgeFeatures" (#151268) Reverts llvm/llvm-project#151107
hoodmane
added a commit
to hoodmane/llvm-project
that referenced
this pull request
Jul 30, 2025
Also alphebetize feature list, add `-mgc` and `-mno-gc` flags, and add some missing feature tests. Reland of llvm#151107. llvm#150201 (comment)
searlmc1
pushed a commit
to ROCm/llvm-project
that referenced
this pull request
Jul 30, 2025
llvm#151107)" breaks tests This reverts commit fe25445.
aheejin
pushed a commit
that referenced
this pull request
Jul 30, 2025
Also alphebetize feature list, add `-mgc` and `-mno-gc` flags, and add some missing feature tests. Reland of #151107. #150201 (comment)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See suggestion here:
#150201 (comment)
cc @sbc100 @dschuff