Skip to content
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

[AMDGPU] Fix a potential wrong return value indicating whether a pass modifies a function #88197

Merged
merged 1 commit into from
Apr 12, 2024

Conversation

shiltian
Copy link
Contributor

@shiltian shiltian commented Apr 9, 2024

When the alloca is too big for vectorization, the function could have already
been modified in previous iteration of the for loop.

@llvmbot
Copy link
Collaborator

llvmbot commented Apr 9, 2024

@llvm/pr-subscribers-backend-amdgpu

Author: Shilei Tian (shiltian)

Changes

When the alloca is too big for vectorization, the function could have already
been modified in previous iteration of the for loop.


Full diff: https://github.com/llvm/llvm-project/pull/88197.diff

1 Files Affected:

  • (modified) llvm/lib/Target/AMDGPU/AMDGPUPromoteAlloca.cpp (+1-1)
diff --git a/llvm/lib/Target/AMDGPU/AMDGPUPromoteAlloca.cpp b/llvm/lib/Target/AMDGPU/AMDGPUPromoteAlloca.cpp
index 6f3cdf54dceec7..c0846b123d1870 100644
--- a/llvm/lib/Target/AMDGPU/AMDGPUPromoteAlloca.cpp
+++ b/llvm/lib/Target/AMDGPU/AMDGPUPromoteAlloca.cpp
@@ -336,7 +336,7 @@ bool AMDGPUPromoteAllocaImpl::run(Function &F, bool PromoteToLDS) {
     if (AllocaCost > VectorizationBudget) {
       LLVM_DEBUG(dbgs() << "  Alloca too big for vectorization: " << *AI
                         << "\n");
-      return false;
+      return Changed;
     }
 
     if (tryPromoteAllocaToVector(*AI)) {

… modifies a function

When the alloca is too big for vectorization, the function could have already
been modified in previous iteration of the `for` loop.
Copy link
Contributor

@arsenm arsenm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also break out of the above loop on the first non-alloca inst

@shiltian shiltian merged commit b4df0da into llvm:main Apr 12, 2024
4 checks passed
@shiltian shiltian deleted the wrong-return branch April 12, 2024 13:34
@shiltian
Copy link
Contributor Author

We should also break out of the above loop on the first non-alloca inst

I'll do it in a follow-up patch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants