This is an archive of the discontinued LLVM Phabricator instance.

[NFC] Combine runNewPMPasses() and runNewPMCustomPasses()
ClosedPublic

Authored by aeubanks on Feb 11 2021, 2:38 PM.

Details

Summary

I've already witnessed two separate changes missing runNewPMPasses()
because runNewPMCustomPasses() is so similar.

This cleans up some duplicated code.

Diff Detail

Event Timeline

aeubanks created this revision.Feb 11 2021, 2:38 PM
aeubanks requested review of this revision.Feb 11 2021, 2:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 11 2021, 2:38 PM
aeubanks updated this revision to Diff 323165.Feb 11 2021, 2:45 PM

formatting

Looks like a great cleanup! One question below.

Looks like a great cleanup! One question below.

I don't see any comments

Looks like a great cleanup! One question below.

I don't see any comments

Weird, I guess I neglected to save it. Added it below.

llvm/lib/LTO/LTOBackend.cpp
211

Will setting the PGOOpt and passing down to the PassBuilder cause an issue for the custom case?

aeubanks added inline comments.Feb 12 2021, 3:48 PM
llvm/lib/LTO/LTOBackend.cpp
211

AFAICT the custom passes is mostly for experimenting with new custom pass pipelines, not the default pipelines that PassBuilder provides. PGOOpt is only used for PassBuilder's default pipelines.
Also there aren't a lot of tests that use it, and I think that the custom pipeline is not really used much in practice.

I guess what I'm saying is I'm not sure if this is really used and I'm don't think that it'll matter a lot.

tejohnson accepted this revision.Feb 12 2021, 4:18 PM

lgtm

llvm/lib/LTO/LTOBackend.cpp
211

Ok. I'm going to assume that if one of these PGO types is enabled in the Config, that it should be honored. And if different behavior is desired, someone should put in a test for it.

This revision is now accepted and ready to land.Feb 12 2021, 4:18 PM
This revision was landed with ongoing or failed builds.Feb 12 2021, 4:45 PM
This revision was automatically updated to reflect the committed changes.

Don't know why no buildbot has caught it, but I think (haven't bisected yet, but this seems like the most likely culprit) this broke

LLVM :: tools/llvm-lto2/X86/pipeline.ll
lld :: ELF/lto/ltopasses-custom.ll

It seems that now, in both of these cases
; CHECK: define void @foo() {

has changed to
define void @foo() local_unnamed_addr #0 {
which is probably a good thing but tests need adjusting.

Don't know why no buildbot has caught it, but I think (haven't bisected yet, but this seems like the most likely culprit) this broke

LLVM :: tools/llvm-lto2/X86/pipeline.ll
lld :: ELF/lto/ltopasses-custom.ll

715dc556b782f718ce1815aaf5c58626f5fe839c should help

Don't know why no buildbot has caught it, but I think (haven't bisected yet, but this seems like the most likely culprit) this broke

LLVM :: tools/llvm-lto2/X86/pipeline.ll
lld :: ELF/lto/ltopasses-custom.ll

715dc556b782f718ce1815aaf5c58626f5fe839c should help

Thanks, that fixed the tests.

Sorry about the breakages, I forgot that some people still have the legacy PM on by default.