This is an archive of the discontinued LLVM Phabricator instance.

[FuncSpec] Enable it only with -O3
ClosedPublic

Authored by SjoerdMeijer on Oct 20 2021, 3:52 AM.

Details

Summary

Looks like function specialisation was running at all optimisation levels (if enabled on the command line, it is not on by default). I think that was an oversight and not something we want to do. Function specialisation duplicates functions when it triggers, so the backend is processing more functions/instructions resulting in compile-time increases, which seems more appropriate with -O3 and inline with GCC.

Please note that since function specialisation is not enabled by default, this didn't require updating any pass manager tests.

Diff Detail

Event Timeline

SjoerdMeijer created this revision.Oct 20 2021, 3:52 AM
SjoerdMeijer requested review of this revision.Oct 20 2021, 3:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 20 2021, 3:52 AM
ChuanqiXu added inline comments.Oct 20 2021, 4:16 AM
llvm/lib/Passes/PassBuilderPipelines.cpp
1409

Do we need to consider SizeLevel here?

Ah, I never pressed the submit button... :-(
See my comment inline.

llvm/lib/Passes/PassBuilderPipelines.cpp
1409

I think this is fine as it is, because if the Speed level is 3, we know we are not optimising for size:

https://github.com/llvm/llvm-project/blob/a3c05982ac05637a8f40207db83c04a1710033d5/llvm/include/llvm/Passes/OptimizationLevel.h#L32

aeubanks added inline comments.Nov 3 2021, 8:34 AM
llvm/lib/Passes/PassBuilderPipelines.cpp
1409

isn't this the same as && Level == OptimizationLevel::O3? any reason why the two checks are slightly different? I think == OptimizationLevel::O3 is a bit more consistent with the other code nearby.

ChuanqiXu accepted this revision.Nov 3 2021, 7:10 PM

LGTM with @aeubanks's the comment addressed.

This revision is now accepted and ready to land.Nov 3 2021, 7:10 PM
SjoerdMeijer added inline comments.Nov 4 2021, 2:34 AM
llvm/lib/Passes/PassBuilderPipelines.cpp
1409

Oh yes, agreed. This was not very intentional, a copy-paste from another place. I will change/fix this before committing. Thanks for spotting this and reviewing.

This revision was automatically updated to reflect the committed changes.