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] Do not preserve UniformityInfo #76696

Merged
merged 3 commits into from
Jan 2, 2024
Merged

[AMDGPU] Do not preserve UniformityInfo #76696

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7806a54
[AMDGPU] Do not preserve UniformityInfo
ssahasra Dec 23, 2023
f235ba1
add generic note about not preserving uniformity analysis
ssahasra Jan 2, 2024
a553f20
minor fix in comment: "updation"
ssahasra Jan 2, 2024
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
6 changes: 6 additions & 0 deletions llvm/include/llvm/ADT/GenericUniformityImpl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,12 @@
/// the propagation of the impact of divergent control flow on the divergence of
/// values (sync dependencies).
///
/// NOTE: In general, no interface exists for a transform to update
/// (Machine)UniformityInfo. Additionally, (Machine)CycleAnalysis is a
/// transitive dependence, but it also does not provide an interface for
/// updating itself. Given that, transforms should not preserve uniformity in
/// their getAnalysisUsage() callback.
///
//===----------------------------------------------------------------------===//

#ifndef LLVM_ADT_GENERICUNIFORMITYIMPL_H
Expand Down
1 change: 0 additions & 1 deletion llvm/lib/Target/AMDGPU/AMDGPURewriteUndefForPHI.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,6 @@ class AMDGPURewriteUndefForPHILegacy : public FunctionPass {
AU.addRequired<DominatorTreeWrapperPass>();

AU.addPreserved<DominatorTreeWrapperPass>();
AU.addPreserved<UniformityInfoWrapperPass>();
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably should add an explanatory comment

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What would be the value of such a comment tucked away in some pass? Why only these specific passes? Why not others? We are just returning to the original state that we don't really know how to preserve UniformityAnalysis in general.

Copy link
Contributor

Choose a reason for hiding this comment

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

Mostly I'm worried about someone getting the idea again that this could be preserved. Feels like it should be documented somewhere, but there isn't a good place. Will anything break in a future change if this is re-added?

Copy link
Contributor

Choose a reason for hiding this comment

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

Will anything break in a future change if this is re-added?

Unfortunately we have never had a good way to diagnose this problem (preserving an analysis but not one of its transitive dependencies). I did once look at adding something to the legacy pass manager to detect it early (when adding passes instead of when running them) but never finished it.

AU.setPreservesCFG();
}
};
Expand Down
3 changes: 0 additions & 3 deletions llvm/lib/Target/AMDGPU/AMDGPUUnifyDivergentExitNodes.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,9 +109,6 @@ void AMDGPUUnifyDivergentExitNodes::getAnalysisUsage(AnalysisUsage &AU) const {
// FIXME: preserve PostDominatorTreeWrapperPass
}

// No divergent values are changed, only blocks and branch edges.
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto

AU.addPreserved<UniformityInfoWrapperPass>();

// We preserve the non-critical-edgeness property
AU.addPreservedID(BreakCriticalEdgesID);

Expand Down