[clang] Increase VecLib bitfield size to 4 bits in CodeGenOptions.def #108804
Conversation
This file contains 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
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Mainak Sil (MainakSil) ChangesSummary: In this PR, I have increased the size of the VecLib bitfield from 3 to 4 to account for the additional libraries. This ensures that all 9 vector libraries are correctly encoded and available for use without errors. Changes Made: Motivation: Closes: Full diff: https://github.com/llvm/llvm-project/pull/108804.diff 1 Files Affected:
diff --git a/clang/include/clang/Basic/CodeGenOptions.def b/clang/include/clang/Basic/CodeGenOptions.def
index b600198998d85b..2d5bb84d1cc59c 100644
--- a/clang/include/clang/Basic/CodeGenOptions.def
+++ b/clang/include/clang/Basic/CodeGenOptions.def
@@ -375,8 +375,13 @@ ENUM_CODEGENOPT(Inlining, InliningMethod, 2, NormalInlining)
/// The maximum stack size a function can have to be considered for inlining.
VALUE_CODEGENOPT(InlineMaxStackSize, 32, UINT_MAX)
+// Ensure the VecLib bitfield has enough space for future vector libraries.
+// If new vector libraries are added beyond the current limit of 16, this static assertion will fail.
+static_assert(static_cast<int>(llvm::driver::VectorLibrary::NoLibrary) <= 16,
+ "The VecLib bitfield in CodeGenOptions needs to be expanded to support more vector libraries.");
+
// Vector functions library to use.
-ENUM_CODEGENOPT(VecLib, llvm::driver::VectorLibrary, 3, llvm::driver::VectorLibrary::NoLibrary)
+ENUM_CODEGENOPT(VecLib, llvm::driver::VectorLibrary, 4, llvm::driver::VectorLibrary::NoLibrary)
/// The default TLS model to use.
ENUM_CODEGENOPT(DefaultTLSModel, TLSModel, 2, GeneralDynamicTLSModel)
|
MainakSil
changed the title
erichkeane
reviewed
Sep 17, 2024
| @@ -375,8 +375,13 @@ ENUM_CODEGENOPT(Inlining, InliningMethod, 2, NormalInlining) | |||
| /// The maximum stack size a function can have to be considered for inlining. | |||
| VALUE_CODEGENOPT(InlineMaxStackSize, 32, UINT_MAX) | |||
|
|
|||
| // Ensure the VecLib bitfield has enough space for future vector libraries. | |||
| // If new vector libraries are added beyond the current limit of 16, this static assertion will fail. | |||
| static_assert(static_cast<int>(llvm::driver::VectorLibrary::NoLibrary) <= 16, | |||
There was a problem hiding this comment.
Hmm... I would very much like to avoid the magic # of 16 here. Does LLVM have any helper libraries for 'get size of bitfield' that we could use? If not, I think we need to be better about the comment, the 16 seems a little 'magic number'y with out some sort of, "As defined by the number of bits in the VecLib CODEGENOPT, defined below" in there.
So I guess I'm asking for either:
1- Make the comment VERY clear for our downstreams that these two are linked
2- (better) find a way to make the 16 get picked up 'automatically' based on the ENUM_CODEGENOPT below.
For 2, I might suggest doing something like:
#define VecLibBits
static_assert(blah blah)
ENUM_CODEGENOPT(blah blah...)
#undef VecLibBits
There was a problem hiding this comment.
Also, should static assert to size_t instead of int, but that is just a nit.
| @@ -375,8 +375,13 @@ ENUM_CODEGENOPT(Inlining, InliningMethod, 2, NormalInlining) | |||
| /// The maximum stack size a function can have to be considered for inlining. | |||
| VALUE_CODEGENOPT(InlineMaxStackSize, 32, UINT_MAX) | |||
|
|
|||
| // Ensure the VecLib bitfield has enough space for future vector libraries. | |||
| // If new vector libraries are added beyond the current limit of 16, this static assertion will fail. | |||
| static_assert(static_cast<int>(llvm::driver::VectorLibrary::NoLibrary) <= 16, | |||
There was a problem hiding this comment.
This doesn't actually protect against anything because NoLibrary is the first enumerator, not the last one.
(Also, I suspect we're missing significant test coverage if we managed to add new libraries and zero tests failed as a result of the too-small bit-field.)
|
Check Please |
You can test this locally with the following command:git-clang-format --diff 2ae968a0d9fb61606b020e898d884c82dd0ed8b5 40d12643aa623e3c8839a9cb90038416d26b8096 --extensions cpp -- clang/unittests/CodeGen/AllLibrariesFit.cpp clang/unittests/CodeGen/EncodingDecodingTest.cpp clang/unittests/CodeGen/SimulatedOverflowTest.cppView the diff from clang-format here.diff --git a/clang/unittests/CodeGen/AllLibrariesFit.cpp b/clang/unittests/CodeGen/AllLibrariesFit.cpp
index dfe63b5577..7bb2891289 100644
--- a/clang/unittests/CodeGen/AllLibrariesFit.cpp
+++ b/clang/unittests/CodeGen/AllLibrariesFit.cpp
@@ -7,4 +7,4 @@ TEST(VecLibBitfieldTest, AllLibrariesFit) {
EXPECT_LE(static_cast<size_t>(llvm::driver::VectorLibrary::MaxLibrary),
(1 << VECLIB_BIT_COUNT))
<< "VecLib bitfield size is too small!";
- }
+}
|
AaronBallman
approved these changes
Sep 18, 2024
erichkeane
approved these changes
Sep 18, 2024
|
If it looks good to you guys, please merge it. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/162/builds/6566 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/78/builds/6198 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/71/builds/6729 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/131/builds/6662 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/89/builds/6624 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/179/builds/6569 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/30/builds/6456 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/6859 Here is the relevant piece of the build log for the reference |
AaronBallman
added a commit
that referenced
this pull request
Sep 18, 2024
…ions.def" (#109161) Reverts #108804 Bots are failing: https://lab.llvm.org/buildbot/#/builders/140/builds/6859
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/2/builds/7016 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/157/builds/8062 Here is the relevant piece of the build log for the reference |
|
I had to revert due to build breakages (linked in comments): I didn't notice that you were missing changes to CMakeLists.txt to add the new files. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/144/builds/7339 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/153/builds/9302 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/56/builds/7718 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/60/builds/7937 Here is the relevant piece of the build log for the reference |
tmsri
pushed a commit
to tmsri/llvm-project
that referenced
this pull request
Sep 19, 2024
…llvm#108804) Summary: This PR fixes the issue where the VecLib bitfield in CodeGenOptions.def is too small to accommodate the increasing number of vector libraries. Specifically, the bitfield size was previously set to 3, but with the introduction of more vector libraries (currently 9), the bitfield needed to be expanded to avoid potential issues in vectorization. In this PR, I have increased the size of the VecLib bitfield from 3 to 4 to account for the additional libraries. This ensures that all 9 vector libraries are correctly encoded and available for use without errors. Changes Made: Modified: Increased the VecLib bitfield size from 3 to 4 in clang/include/clang/Basic/CodeGenOptions.def. Motivation: This change is necessary to ensure that all vector libraries are properly represented and selectable. The current limitation of the VecLib bitfield size was causing some vectorization opportunities to be lost when more than 3 bits were needed to represent the library options. Closes: Fixes llvm#108704
tmsri
pushed a commit
to tmsri/llvm-project
that referenced
this pull request
Sep 19, 2024
…ions.def" (llvm#109161) Reverts llvm#108804 Bots are failing: https://lab.llvm.org/buildbot/#/builders/140/builds/6859
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary:
This PR fixes the issue where the VecLib bitfield in CodeGenOptions.def is too small to accommodate the increasing number of vector libraries. Specifically, the bitfield size was previously set to 3, but with the introduction of more vector libraries (currently 9), the bitfield needed to be expanded to avoid potential issues in vectorization.
In this PR, I have increased the size of the VecLib bitfield from 3 to 4 to account for the additional libraries. This ensures that all 9 vector libraries are correctly encoded and available for use without errors.
Changes Made:
Modified: Increased the VecLib bitfield size from 3 to 4 in clang/include/clang/Basic/CodeGenOptions.def.
Motivation:
This change is necessary to ensure that all vector libraries are properly represented and selectable. The current limitation of the VecLib bitfield size was causing some vectorization opportunities to be lost when more than 3 bits were needed to represent the library options.
Closes:
Fixes #108704