This is an archive of the discontinued LLVM Phabricator instance.

[JumpThreading][NFC][CompileTime] Do not recompute BPI/BFI analyzes
ClosedPublic

Authored by mkazantsev on Apr 26 2022, 1:05 AM.

Details

Summary

They can already be available, and even if not, DT/LI can be available. We should not recompute them.
Old PM is unchanged because it would require changing dependencies, and we don't care enough about it.

Diff Detail

Event Timeline

mkazantsev created this revision.Apr 26 2022, 1:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 26 2022, 1:05 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
mkazantsev requested review of this revision.Apr 26 2022, 1:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 26 2022, 1:05 AM

My measurements are tad noisy, but they show ~10% JumpThreading CT saving on a big method.

mkazantsev retitled this revision from [JumpThreading][NFC] Reuse existing LoopInfo instead of recomputation to [JumpThreading][NFC][CompileTime] Reuse existing LoopInfo instead of recomputation.Apr 26 2022, 1:14 AM
nikic added inline comments.Apr 26 2022, 8:59 AM
llvm/lib/Transforms/Scalar/JumpThreading.cpp
327

You'd have to also register it with AnalysisUses and initialize a pass dependency. I'd suggest to leave the LegacyPM implementation alone.

aeubanks added inline comments.
llvm/lib/Transforms/Scalar/JumpThreading.cpp
353–355

can these two also go through the analysis manager?

mkazantsev added inline comments.Apr 27 2022, 5:38 AM
llvm/lib/Transforms/Scalar/JumpThreading.cpp
327

You're right.

mkazantsev marked 2 inline comments as done.Apr 27 2022, 6:01 AM
mkazantsev retitled this revision from [JumpThreading][NFC][CompileTime] Reuse existing LoopInfo instead of recomputation to [JumpThreading][NFC][CompileTime] Do not recompute BPI/BFI analyzes.
mkazantsev edited the summary of this revision. (Show Details)
mkazantsev added a reviewer: aeubanks.

Addressed comments.

This revision is now accepted and ready to land.Apr 27 2022, 6:45 AM
aeubanks accepted this revision.Apr 27 2022, 9:10 AM
This revision was landed with ongoing or failed builds.Apr 27 2022, 9:17 PM
This revision was automatically updated to reflect the committed changes.

This change has caused our internal PGO+NewPM builds of clang to have become non-reproducible (that is: the binary is different every build, when built with a Clang that includes this change). The problem appears only when providing profile data. I am not sure why this change causes a problem -- perhaps some of the analysis data ought to have been invalidated by some other pass, but wasn't?

But, I've bisected to exactly this commit, so, I'm going to revert it while continuing to investigate.

ebrevnov added a subscriber: ebrevnov.EditedAug 18 2022, 2:17 AM

This change has caused our internal PGO+NewPM builds of clang to have become non-reproducible (that is: the binary is different every build, when built with a Clang that includes this change). The problem appears only when providing profile data. I am not sure why this change causes a problem -- perhaps some of the analysis data ought to have been invalidated by some other pass, but wasn't?

But, I've bisected to exactly this commit, so, I'm going to revert it while continuing to investigate.

Hi! Any update on this?

Not sure if it has any impact on your case but there is a chance that JumpThreading updates profile metadata through updatePredecessorProfileMetadata and doesn't invalidate BPI/BFI analysis.

This change has caused our internal PGO+NewPM builds of clang to have become non-reproducible (that is: the binary is different every build, when built with a Clang that includes this change). The problem appears only when providing profile data. I am not sure why this change causes a problem -- perhaps some of the analysis data ought to have been invalidated by some other pass, but wasn't?

But, I've bisected to exactly this commit, so, I'm going to revert it while continuing to investigate.

Hi! Any update on this?

Not sure if it has any impact on your case but there is a chance that JumpThreading updates profile metadata through updatePredecessorProfileMetadata and doesn't invalidate BPI/BFI analysis.

@jyknight, ping