-
Notifications
You must be signed in to change notification settings - Fork 15.1k
[InstCombine] visitShuffleVectorInst assert with vector of pointers fix. #152341
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
@llvm/pr-subscribers-llvm-transforms Author: Szymon Piotr Milczek (smilczek) ChangesIn visitShuffleVectorInst there's an if block that's meant to turn It assumes that there will never be bitcasts performed on vectors of ptr There is however an edge case where a bitcast instruction can be In this edge case, the code initializes the variable This commit adds a condition to the if statement, that ensures the logic Full diff: https://github.com/llvm/llvm-project/pull/152341.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineVectorOps.cpp b/llvm/lib/Transforms/InstCombine/InstCombineVectorOps.cpp
index fe0f308223387..510c2cf0bed04 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineVectorOps.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineVectorOps.cpp
@@ -3038,7 +3038,8 @@ Instruction *InstCombinerImpl::visitShuffleVectorInst(ShuffleVectorInst &SVI) {
// Index range [6,10): ^-----------^ Needs an extra shuffle.
// Target type i40: ^--------------^ Won't work, bail.
bool MadeChange = false;
- if (isShuffleExtractingFromLHS(SVI, Mask)) {
+ if (isShuffleExtractingFromLHS(SVI, Mask) &&
+ !LHS->getType()->isPtrOrPtrVectorTy()) {
Value *V = LHS;
unsigned MaskElems = Mask.size();
auto *SrcTy = cast<FixedVectorType>(V->getType());
diff --git a/llvm/test/Transforms/InstCombine/2025-08-06-shufflevector-bitcast-vector-of-pointers.ll b/llvm/test/Transforms/InstCombine/2025-08-06-shufflevector-bitcast-vector-of-pointers.ll
new file mode 100644
index 0000000000000..a035dfbfceaa0
--- /dev/null
+++ b/llvm/test/Transforms/InstCombine/2025-08-06-shufflevector-bitcast-vector-of-pointers.ll
@@ -0,0 +1,9 @@
+; RUN: opt < %s -passes=instcombine -S
+
+; Make sure that we don't crash when optimizing shufflevector of <N x ptr> with <1 x i32> mask followed by bitcast of <1 x ptr> to ptr
+
+define ptr @test(<3 x ptr> %vptr) {
+ %SV = shufflevector <3 x ptr> %vptr, <3 x ptr> zeroinitializer, <1 x i32> zeroinitializer
+ %BC = bitcast <1 x ptr> %SV to ptr
+ ret ptr %BC
+}
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can still fold this pattern into extractelement <3 x ptr> %vptr, i64 0
.
It would be better to fix the VecBitWidth calculation than to bail out on pointer vector types.
unsigned VecBitWidth = SrcTy->getPrimitiveSizeInBits().getFixedValue();
-> unsigned VecBitWidth = DL.getTypeSizeInBits(SrcTy);
Can you test if this change works?
llvm/test/Transforms/InstCombine/2025-08-06-shufflevector-bitcast-vector-of-pointers.ll
Outdated
Show resolved
Hide resolved
@dtcxzyw done in 6786ea40f92b1e83b3b6e6b239d77147f81e4c82 |
@@ -0,0 +1,10 @@ | |||
; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5 | |||
; RUN: opt < %s -passes=instcombine -S |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
; RUN: opt < %s -passes=instcombine -S | |
; RUN: opt < %s -passes=instcombine -S | FileCheck %s |
Otherwise check lines will not be generated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in 233212fc56ef2ed08605039a39fab52a9f943a67
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
233212f
to
afffe6f
Compare
In visitShuffleVectorInst there's an if block that's meant to turn shufflevector followed by bitcast into extractelement where possible. It assumes that there will never be bitcasts performed on vectors of ptr as such operations are almost always illegal, and ptrtoint instructions should be used instead. There is however an edge case where a bitcast instruction can be performed on a vector of type `<1 x ptr>` to turn it into type `ptr` In this edge case, the code initializes the variable `VecBitWidth` to 0. Then, when iterating over users that are bitcasts, an attempt is made to create a vector of size 0, which triggers and assert. This commit changes initialization of `VecBitWidth` to use datalayout to find the the size of the vector instead of getPrimitiveSizeInBits method which results in 0 for ptr and vectors of ptr.
afffe6f
to
4d36858
Compare
@smilczek Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR. Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues. How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
In visitShuffleVectorInst there's an if block that's meant to turn
shufflevector followed by bitcast into extractelement where possible.
It assumes that there will never be bitcasts performed on vectors of ptr
as such operations are almost always illegal, and ptrtoint instructions
should be used instead.
There is however an edge case where a bitcast instruction can be
performed on a vector of type
<1 x ptr>
to turn it into typeptr
In this edge case, the code initializes the variable
VecBitWidth
to 0.Then, when iterating over users that are bitcasts, an attempt is made to
create a vector of size 0, which triggers and assert.
This commit changes initialization of
VecBitWidth
to use datalayout tofind the the size of the vector instead of getPrimitiveSizeInBits method
which results in 0 for ptr and vectors of ptr.