-
Notifications
You must be signed in to change notification settings - Fork 14.4k
[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
Conversation
@llvm/pr-subscribers-backend-amdgpu Author: Chengjun (Chengjunp) ChangesIn current This patch adds a check to ensure we only take AddressSpace on a pointer value. If either of Full diff: https://github.com/llvm/llvm-project/pull/128116.diff 1 Files Affected:
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;
|
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.
Needs test
if (!TypeA->isPointerTy() || !TypeB->isPointerTy()) | ||
return AliasResult::MayAlias; | ||
unsigned asA = TypeA->getPointerAddressSpace(); | ||
unsigned asB = TypeB->getPointerAddressSpace(); |
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.
use dyn_cast to PointerType
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.
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 |
In current
AMDGPUAAResult::alias
, we directly take the AddressSpace ofLocA.Ptr->getType()
. It's unsafe asLocA.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
orLocB.Ptr
is non-pointer, we returnAliasResult::MayAlias
which is the most conservative result.