This is an archive of the discontinued LLVM Phabricator instance.

[LTO] Expose opt() in LTOBackend (NFC).
ClosedPublic

Authored by fhahn on Jan 12 2021, 3:25 AM.

Details

Summary

Exposing opt() which runs middle-end LTO optimzation allows re-using it
in LTOCodeGenerator.

Diff Detail

Event Timeline

fhahn created this revision.Jan 12 2021, 3:25 AM
fhahn requested review of this revision.Jan 12 2021, 3:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 12 2021, 3:25 AM

LGTM with a small comment.

llvm/lib/LTO/LTOBackend.cpp
518

Any reason why the function need to be moved instead of just renamed? I will reduce the diff if possible.

fhahn added inline comments.Jan 12 2021, 12:09 PM
llvm/lib/LTO/LTOBackend.cpp
518

Unfortunately it is originally defined in a anonymous namespace, so I think it has to be moved out of that to be defined in lto::.

dexonsmith added inline comments.Jan 12 2021, 4:38 PM
llvm/lib/LTO/LTOBackend.cpp
518

[drive-by comments]

I think just adding the lto:: should do the right thing. If not, you can temporarily close the anonymous namespace and reopen it.

Another option is an NFC prep commit that limits the anonymous namespace to .cpp-declared structures, and adds a static keyword on each function that's no longer "anonymous". That'd match the more typical LLVM style... reducing this patch to s/static bool opt/bool lto::opt/.

fhahn updated this revision to Diff 316352.Jan 13 2021, 2:52 AM

Rebased after f638c2eb4ee6d0a0bd0e80cd305ad93e382db8f5, which removed the anonymous namespace (it just included function definitions), thanks for the tip!

Now the diff boils down to just changing the name.

fhahn added inline comments.Jan 13 2021, 2:54 AM
llvm/lib/LTO/LTOBackend.cpp
518

Another option is an NFC prep commit that limits the anonymous namespace to .cpp-declared structures, and adds a static keyword on each function that's no longer "anonymous". That'd match the more typical LLVM style... reducing this patch to s/static bool opt/bool lto::opt/.

Ah yes, LLVM style suggests to only use anonymous namespaces for class declarations! I went with this suggestion and removed the anonymous namespace in f638c2eb4ee6d0a0bd0e80cd305ad93e382db8f5, because it just contained function definitions.

This revision is now accepted and ready to land.Jan 13 2021, 10:21 AM
This revision was landed with ongoing or failed builds.Jan 14 2021, 2:07 AM
This revision was automatically updated to reflect the committed changes.