This is an archive of the discontinued LLVM Phabricator instance.

[NFC] Rename ThinLTOPhase to PhaseInAllLTO and move it from PassBuilder.h to Pass.h
ClosedPublic

Authored by wmi on Jan 13 2021, 9:59 AM.

Details

Summary

In some compiler passes like SampleProfileLoaderPass, we want to know which LTO/ThinLTO phase the pass is in. Currently the phase is represented in enum class PassBuilder::ThinLTOPhase, so it is only available in PassBuilder and it also cannot represent phase in full LTO. The patch extends it to include LTO phases and move it from PassBuilder.h to Pass.h, then it is much easier for PassBuilder to communiate with each pass about current LTO phase.

Diff Detail

Repository
rL LLVM

Event Timeline

wmi created this revision.Jan 13 2021, 9:59 AM
wmi requested review of this revision.Jan 13 2021, 9:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 13 2021, 9:59 AM

Thanks for refactoring. We had internal change to add IsFullLTOPreLink alongside with IsThinLTOPreLink for a few places. And we always wanted clean it up too. :)

llvm/include/llvm/Pass.h
75

nit: name it LTOPhase? There's similar change in https://reviews.llvm.org/D69732.

And perhaps FullLTOPreLink and FullLTOPostLink too though I noticed there's PrepareForLTO and PrepareForThinLTO.. Not sure if LTO is always used to refer FullLTO..

davidxl added inline comments.Jan 13 2021, 11:49 AM
llvm/include/llvm/Pass.h
75

LTO traditionally refers to FullLTO, so using LTO can be confusing.

I suggest make it even clearer:

ThinOrFullLTOPhase

wmi updated this revision to Diff 316508.Jan 13 2021, 1:55 PM

Address Wenlei and David's comments.

wmi added inline comments.Jan 13 2021, 1:56 PM
llvm/include/llvm/Pass.h
75

Thanks, I use ThinOrFullLTOPhase for the enum class name and change LTOPreLink/LTOPostLink to FullLTOPreLink/FullLTOPostLink.

wenlei accepted this revision.Jan 13 2021, 1:58 PM

lgtm

This revision is now accepted and ready to land.Jan 13 2021, 1:58 PM
hoy added a comment.EditedJan 13 2021, 2:14 PM

Thanks for the refactoring!

BTW, looks like there is a use of ThinLTOPhase::None to be replaced in polly/lib/Support/RegisterPasses.cpp. Otherwise, LGTM.

hoy accepted this revision.Jan 13 2021, 2:14 PM
wmi added a comment.Jan 13 2021, 3:06 PM
In D94613#2496792, @hoy wrote:

Thanks for the refactoring!

BTW, looks like there is a use of ThinLTOPhase::None to be replaced in polly/lib/Support/RegisterPasses.cpp. Otherwise, LGTM.

Ah, Good catch, thanks!