Skip to content

Clean up strange uses of getAnalysisIfAvailable #65729

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

Merged
merged 2 commits into from
Oct 11, 2023
Merged

Clean up strange uses of getAnalysisIfAvailable #65729

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
66129ac
Clean up strange uses of getAnalysisIfAvailable
jayfoad Sep 8, 2023
0fec568
Merge remote-tracking branch 'origin/main' into getanalysisifavailable
jayfoad Oct 11, 2023
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
4 changes: 2 additions & 2 deletions llvm/lib/CodeGen/EarlyIfConversion.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1092,7 +1092,7 @@ bool EarlyIfConverter::runOnMachineFunction(MachineFunction &MF) {
SchedModel = STI.getSchedModel();
MRI = &MF.getRegInfo();
DomTree = &getAnalysis<MachineDominatorTree>();
Loops = getAnalysisIfAvailable<MachineLoopInfo>();
Loops = &getAnalysis<MachineLoopInfo>();
Traces = &getAnalysis<MachineTraceMetrics>();
MinInstr = nullptr;

Expand Down Expand Up @@ -1226,7 +1226,7 @@ bool EarlyIfPredicator::runOnMachineFunction(MachineFunction &MF) {
MRI = &MF.getRegInfo();
SchedModel.init(&STI);
DomTree = &getAnalysis<MachineDominatorTree>();
Loops = getAnalysisIfAvailable<MachineLoopInfo>();
Loops = &getAnalysis<MachineLoopInfo>();
MBPI = &getAnalysis<MachineBranchProbabilityInfo>();

bool Changed = false;
Expand Down
7 changes: 2 additions & 5 deletions llvm/lib/CodeGen/TypePromotion.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1016,11 +1016,8 @@ bool TypePromotionLegacy::runOnFunction(Function &F) {
if (skipFunction(F))
return false;

auto *TPC = getAnalysisIfAvailable<TargetPassConfig>();
if (!TPC)
return false;

auto *TM = &TPC->getTM<TargetMachine>();
auto &TPC = getAnalysis<TargetPassConfig>();
auto *TM = &TPC.getTM<TargetMachine>();
auto &TTI = getAnalysis<TargetTransformInfoWrapperPass>().getTTI(F);
auto &LI = getAnalysis<LoopInfoWrapperPass>().getLoopInfo();

Expand Down
2 changes: 1 addition & 1 deletion llvm/lib/CodeGen/VirtRegMap.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,7 +261,7 @@ bool VirtRegRewriter::runOnMachineFunction(MachineFunction &fn) {
Indexes = &getAnalysis<SlotIndexes>();
LIS = &getAnalysis<LiveIntervals>();
VRM = &getAnalysis<VirtRegMap>();
DebugVars = getAnalysisIfAvailable<LiveDebugVariables>();
DebugVars = &getAnalysis<LiveDebugVariables>();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure how to add a comment on line 278 if I want to suggest an edit on that line (in Phabricator it would have been easy). But I guess the check if DebugVars is null or not on line 278 is redundant now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Fixed in 05c16f4

LLVM_DEBUG(dbgs() << "********** REWRITE VIRTUAL REGISTERS **********\n"
<< "********** Function: " << MF->getName() << '\n');
LLVM_DEBUG(VRM->dump());
Expand Down
2 changes: 1 addition & 1 deletion llvm/lib/Target/AArch64/AArch64ConditionalCompares.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -935,7 +935,7 @@ bool AArch64ConditionalCompares::runOnMachineFunction(MachineFunction &MF) {
SchedModel = MF.getSubtarget().getSchedModel();
MRI = &MF.getRegInfo();
DomTree = &getAnalysis<MachineDominatorTree>();
Loops = getAnalysisIfAvailable<MachineLoopInfo>();
Loops = &getAnalysis<MachineLoopInfo>();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think there are some "weird" things going on here. But maybe not worth digging into those unless being an AArch64 maintainer.

Afaict the goal here is to make sure MachineLoopInfo is preserved. So this pass shouldn't really need to require it explicitly. However, the MachineTraceMetrics pass is also using MachineLoopInfo transitively. So, the MachineLoopInfo analysis will be run. Although I think there is a "bug" in MachineTraceMetrics related to not using addRequiredTransitive. So things might break if removing the addRequired in this pass.

To sum up. The proposed change is probably harmless and OK. Just saying that the analysis pass dependencies used in here seem to be a bit ad-hoc(?).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Everything seems to work if I change AArch64ConditionalCompares to not require MachineLoopInfo. But I think I'll leave it to the AArch64 maintainers like you suggest.

MBPI = &getAnalysis<MachineBranchProbabilityInfo>();
Traces = &getAnalysis<MachineTraceMetrics>();
MinInstr = nullptr;
Expand Down
4 changes: 2 additions & 2 deletions llvm/lib/Transforms/Scalar/GVN.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3286,7 +3286,7 @@ class llvm::gvn::GVNLegacyPass : public FunctionPass {
if (skipFunction(F))
return false;

auto *LIWP = getAnalysisIfAvailable<LoopInfoWrapperPass>();
auto &LIWP = getAnalysis<LoopInfoWrapperPass>();
Copy link
Collaborator

Choose a reason for hiding this comment

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

IIUC GVN doesn't depend on LoopInfo itself. It should just make sure to preserve it if it is available. So one might wonder if it is the addRequire that should be questioned and possibly removed instead.
But it's been like this for years (that it is required), and messing around with the legacy PM analysis dependencies for an IR pass (even if it is used by some backends) is perhaps not worth it.

Therefore I think you proposed change is ok. Or you could just leave this as is, since the IfAvailable part in some sense indicate that the pass itself doesn't depend on the analysis.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefer not to leave it as is - I think either we should get the analysis with getAnalysis or we should remove the addRequired, so that they are consistent with each other.


auto *MSSAWP = getAnalysisIfAvailable<MemorySSAWrapperPass>();
return Impl.runImpl(
Expand All @@ -3297,7 +3297,7 @@ class llvm::gvn::GVNLegacyPass : public FunctionPass {
Impl.isMemDepEnabled()
? &getAnalysis<MemoryDependenceWrapperPass>().getMemDep()
: nullptr,
LIWP ? &LIWP->getLoopInfo() : nullptr,
&LIWP.getLoopInfo(),
&getAnalysis<OptimizationRemarkEmitterWrapperPass>().getORE(),
MSSAWP ? &MSSAWP->getMSSA() : nullptr);
}
Expand Down