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.

Sign up for GitHub

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][Attributor] Check the validity of a dependent AA before using its value #114165

Merged
merged 1 commit into from
Oct 30, 2024
Merged

[AMDGPU][Attributor] Check the validity of a dependent AA before using its value #114165

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 12 additions & 8 deletions llvm/lib/Target/AMDGPU/AMDGPUAttributor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -358,7 +358,7 @@ struct AAUniformWorkGroupSizeFunction : public AAUniformWorkGroupSize {

const auto *CallerInfo = A.getAAFor<AAUniformWorkGroupSize>(
*this, IRPosition::function(*Caller), DepClassTy::REQUIRED);
if (!CallerInfo)
if (!CallerInfo || !CallerInfo->isValidState())
return false;

Change = Change | clampStateAndIndicateChange(this->getState(),
Expand Down Expand Up @@ -449,7 +449,8 @@ struct AAAMDAttributesFunction : public AAAMDAttributes {
// Check for Intrinsics and propagate attributes.
const AACallEdges *AAEdges = A.getAAFor<AACallEdges>(
*this, this->getIRPosition(), DepClassTy::REQUIRED);
if (!AAEdges || AAEdges->hasNonAsmUnknownCallee())
if (!AAEdges || !AAEdges->isValidState() ||
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm still not sure when these getAAFors can actually fail

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the IR position is not valid for the queried AA, it can fail.

Copy link
Contributor

Choose a reason for hiding this comment

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

But for all of these function only attributes, how could that happen?

Copy link
Contributor Author

@shiltian shiltian Oct 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

For those AAs here (in this file), it might not fail. There are generally three cases: 1) invalid IR position 2) filtered out AA (not in allow list) 3) function AA with optnone.

AAEdges->hasNonAsmUnknownCallee())
return indicatePessimisticFixpoint();

bool IsNonEntryFunc = !AMDGPU::isEntryFunctionCC(F->getCallingConv());
Expand All @@ -465,7 +466,7 @@ struct AAAMDAttributesFunction : public AAAMDAttributes {
if (IID == Intrinsic::not_intrinsic) {
const AAAMDAttributes *AAAMD = A.getAAFor<AAAMDAttributes>(
*this, IRPosition::function(*Callee), DepClassTy::REQUIRED);
if (!AAAMD)
if (!AAAMD || !AAAMD->isValidState())
return indicatePessimisticFixpoint();
*this &= *AAAMD;
continue;
Expand Down Expand Up @@ -660,7 +661,7 @@ struct AAAMDAttributesFunction : public AAAMDAttributes {

const auto *PointerInfoAA = A.getAAFor<AAPointerInfo>(
*this, IRPosition::callsite_returned(Call), DepClassTy::REQUIRED);
if (!PointerInfoAA)
if (!PointerInfoAA || !PointerInfoAA->getState().isValidState())
return false;

return PointerInfoAA->forallInterferingAccesses(
Expand Down Expand Up @@ -717,7 +718,7 @@ struct AAAMDSizeRangeAttribute

const auto *CallerInfo = A.getAAFor<AttributeImpl>(
*this, IRPosition::function(*Caller), DepClassTy::REQUIRED);
if (!CallerInfo)
if (!CallerInfo || !CallerInfo->isValidState())
return false;

Change |=
Expand Down Expand Up @@ -835,7 +836,8 @@ struct AAAMDWavesPerEU : public AAAMDSizeRangeAttribute {
auto &InfoCache = static_cast<AMDGPUInformationCache &>(A.getInfoCache());

if (const auto *AssumedGroupSize = A.getAAFor<AAAMDFlatWorkGroupSize>(
*this, IRPosition::function(*F), DepClassTy::REQUIRED)) {
*this, IRPosition::function(*F), DepClassTy::REQUIRED);
AssumedGroupSize->isValidState()) {

unsigned Min, Max;
std::tie(Min, Max) = InfoCache.getWavesPerEU(
Expand Down Expand Up @@ -864,7 +866,8 @@ struct AAAMDWavesPerEU : public AAAMDSizeRangeAttribute {
*this, IRPosition::function(*Caller), DepClassTy::REQUIRED);
const auto *AssumedGroupSize = A.getAAFor<AAAMDFlatWorkGroupSize>(
*this, IRPosition::function(*Func), DepClassTy::REQUIRED);
if (!CallerInfo || !AssumedGroupSize)
if (!CallerInfo || !AssumedGroupSize || !CallerInfo->isValidState() ||
!AssumedGroupSize->isValidState())
return false;

unsigned Min, Max;
Expand Down Expand Up @@ -982,7 +985,8 @@ struct AAAMDGPUNoAGPR
// TODO: Handle callsite attributes
const auto *CalleeInfo = A.getAAFor<AAAMDGPUNoAGPR>(
*this, IRPosition::function(*Callee), DepClassTy::REQUIRED);
return CalleeInfo && CalleeInfo->getAssumed();
return CalleeInfo && CalleeInfo->isValidState() &&
CalleeInfo->getAssumed();
};

bool UsedAssumedInformation = false;
Expand Down
Loading