Skip to content

[AMDGPUAA] Check Type before Taking AddressSpace of a Value #128116

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

Closed
wants to merge 1 commit into from

Conversation

Chengjunp
Copy link
Contributor

In current AMDGPUAAResult::alias, we directly take the AddressSpace of LocA.Ptr->getType(). It's unsafe as LocA.Ptr->getType() may be a non-pointer type.

This patch adds a check to ensure we only take AddressSpace on a pointer value. If either of LocA.Ptr or LocB.Ptr is non-pointer, we return AliasResult::MayAlias which is the most conservative result.

@llvmbot
Copy link
Member

llvmbot commented Feb 21, 2025

@llvm/pr-subscribers-backend-amdgpu

Author: Chengjun (Chengjunp)

Changes

In current AMDGPUAAResult::alias, we directly take the AddressSpace of LocA.Ptr->getType(). It's unsafe as LocA.Ptr->getType() may be a non-pointer type.

This patch adds a check to ensure we only take AddressSpace on a pointer value. If either of LocA.Ptr or LocB.Ptr is non-pointer, we return AliasResult::MayAlias which is the most conservative result.


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

1 Files Affected:

  • (modified) llvm/lib/Target/AMDGPU/AMDGPUAliasAnalysis.cpp (+6-2)
diff --git a/llvm/lib/Target/AMDGPU/AMDGPUAliasAnalysis.cpp b/llvm/lib/Target/AMDGPU/AMDGPUAliasAnalysis.cpp
index 5a6868f96d970..d60dc22f1d837 100644
--- a/llvm/lib/Target/AMDGPU/AMDGPUAliasAnalysis.cpp
+++ b/llvm/lib/Target/AMDGPU/AMDGPUAliasAnalysis.cpp
@@ -49,8 +49,12 @@ void AMDGPUAAWrapperPass::getAnalysisUsage(AnalysisUsage &AU) const {
 AliasResult AMDGPUAAResult::alias(const MemoryLocation &LocA,
                                   const MemoryLocation &LocB, AAQueryInfo &AAQI,
                                   const Instruction *) {
-  unsigned asA = LocA.Ptr->getType()->getPointerAddressSpace();
-  unsigned asB = LocB.Ptr->getType()->getPointerAddressSpace();
+  Type *TypeA = LocA.Ptr->getType();
+  Type *TypeB = LocB.Ptr->getType();
+  if (!TypeA->isPointerTy() || !TypeB->isPointerTy())
+    return AliasResult::MayAlias;
+  unsigned asA = TypeA->getPointerAddressSpace();
+  unsigned asB = TypeB->getPointerAddressSpace();
 
   if (!AMDGPU::addrspacesMayAlias(asA, asB))
     return AliasResult::NoAlias;

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.

Needs test

Comment on lines +54 to +57
if (!TypeA->isPointerTy() || !TypeB->isPointerTy())
return AliasResult::MayAlias;
unsigned asA = TypeA->getPointerAddressSpace();
unsigned asB = TypeB->getPointerAddressSpace();
Copy link
Contributor

Choose a reason for hiding this comment

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

use dyn_cast to PointerType

Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

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

Ideally we would not allow alias() to be called with non-pointers in the first place. Do you know where these come from?

@Chengjunp
Copy link
Contributor Author

Ideally we would not allow alias() to be called with non-pointers in the first place. Do you know where these come from?

Hi @nikic and @arsenm, this was first found when I tried to move all the target specific AA before BasicAA and then a test for AMDGPU backend crashed here when taking the address space of a non-pointer value. After investigation, the actual problem came from a wrong use of alias check in FlattenCFG, and I'm working on a fix here.

With that fix, actually we do not need to make modification here. Maybe we can just add an assertion to make sure only pointer values are passes to alias function?

@Chengjunp Chengjunp closed this Mar 12, 2025
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.

4 participants