Page MenuHomePhabricator

Enable MachineOutliner by default under -Oz for AArch64
ClosedPublic

Authored by paquette on Apr 20 2018, 6:46 PM.

Details

Summary

This patch enables the MachineOutliner by default in AArch64 under -Oz.

The MachineOutliner offers around a 4.5% improvement on the current -Oz code size improvements.

We have done work into improving the debuggability of outlined code, so that users of -Oz won't be surprised by the optimization. We have also been executing the LLVM test suite and common external tests such as the SPEC suites continuously with no issue. The outliner has a low compile-time overhead of roughly 1%. At this point, the outliner would be a really good addition to the -Oz pass pipeline!

Diff Detail

Event Timeline

paquette created this revision.Apr 20 2018, 6:46 PM
paquette edited the summary of this revision. (Show Details)Apr 20 2018, 6:59 PM
paquette added a subscriber: llvm-commits.

Could you please generate diff with -U999 to give more context to the patch. Thanks.

paquette updated this revision to Diff 143583.Apr 23 2018, 9:48 AM

Added context to diff.

efriedma added inline comments.
lib/CodeGen/TargetPassConfig.cpp
120 ↗(On Diff #143583)

Instead of having two flags, can you just check getNumOccurrences()?

paquette updated this revision to Diff 144158.Apr 26 2018, 10:42 AM

Updated diff to reflect review comments and discussion on the RFC. In the RFC, we decided that it'd be better to base the decision to outline off of function attributes for compatibility with LTO and extensibility of the outliner. This is a far less invasive change, and means that every patch other than this can be abandoned.

I also added a comment on the "-enable-machine-outliner" flag saying what it's *actually* supposed to mean after the application of this patch. In a follow-up patch, we should change this flag to something that better implies what the flag actually does. The only reason I didn't do that here is that I'd have to update every test in *this* patch which is a little too much noise for review.

paquette marked an inline comment as done.Apr 26 2018, 10:45 AM
paquette added inline comments.
lib/CodeGen/TargetPassConfig.cpp
120 ↗(On Diff #143583)

"enable-machine-outliner" implies "run it on everything, even things the target doesn't want by default". This isn't actually symmetric with "disable-machine-outliner", which says "don't run it on anything at all". I think "enable-machine-outliner" should be renamed to something that better describes the intended behaviour. I'll do that in a follow-up patch.

efriedma added inline comments.Apr 27 2018, 2:10 PM
lib/CodeGen/TargetPassConfig.cpp
904 ↗(On Diff #144158)

We probably need to check getOptLevel() != CodeGenOpt::None here.

120 ↗(On Diff #143583)

In other places, we sometimes do something like "true is always, false is never, unspecified is the default". But I guess this isn't really the same thing.

Could you rename the flags to be more clear? (-disable-machine-outliner-pass vs. -machine-outliner-all-functions).

paquette updated this revision to Diff 144746.May 1 2018, 10:16 AM
paquette marked an inline comment as done.

Updated patch for review. Lots of tests modified this time because in this round the outliner is *always* added to the pass pipleline. It then decides whether or not it should run based off of user input/target default behaviour.

Also all the outliner tests have been updated because -enable-machine-outliner now accepts 3 choices: always, default, and never.

-enable-machine-outliner=always => Run the outliner on every function (modulo those with linkonceodr linkage)
-enable-machine-outliner=default => Run the outliner on functions the target decides should always be outlined from (e.g, in this case, those with the minsize attribute in AArch64)
-enable-machine-outliner=never => Do nothing and return false

paquette marked an inline comment as done.May 1 2018, 10:16 AM
paquette added inline comments.
lib/CodeGen/TargetPassConfig.cpp
120 ↗(On Diff #143583)

Done for the current version of the patch

paquette marked 2 inline comments as done.May 1 2018, 10:17 AM
paquette edited the summary of this revision. (Show Details)

Funnily enough, this version of the patch uncovered a bug in a different pass, which caused its lit tests to fail (D46330).

Since I just committed the fix in r331307, this should pass every lit test.

srhines added a subscriber: srhines.May 8 2018, 5:03 PM

The code in this patch looks fine.

In terms of the policy decision of actually turning it on by default, I'm mostly comfortable with it. Outlining has gotten a reasonable amount of testing, and I think the AArch64 hooks are correct. One potential issue which I still need to look into is whether we need to disable outlining for fragments which use r16 and r17; it would be rare and difficult to reproduce, but the linker could theoretically insert a veneer that clobbers those registers in some cases.

yroux added a subscriber: yroux.May 24 2018, 10:33 AM

Hi Jessica,

I passed your patch into our pre-commit validation (on x86_64, AArch64 and ARM boxes) and besides the 2 failures on the recently added AArch64 outliner testcases (which need to be updated) I got regressions on AMDGPU when -march=5600 is used and on PowerPC when mcpu=pwr7 is used without -mattr=-crbits, and I've troubles to figure out why...

Here are the logs for x86: https://ci.linaro.org/job/tcwg-llvm-staged-build/140/label=tcwg-x86_64-cam,target=tcwg-x86_64-cam/console

paquette updated this revision to Diff 152375.Jun 21 2018, 2:01 PM

Updated the patch so that it no longer causes issues for the AMDGPU target. The outliner is now only added to the pass pipeline if a target specifies that it supports it in its TargetOptions. After this, whether or not a target supports the outliner must be defined within a specific TargetMachine (similar to GlobalIsel).

It seems like although the outliner wasn't actually *doing* anything in the AMDGPU target, the verifier was running on the module after it was added, raising issues there.

This is getting complicated enough that I'd like to see it split into two patches; one for the MachineOutliner option/configuration changes, and one to actually change the default for AArch64.

Yeah this is too big. I'll split this up into a few patches then update this one to just reflect turning the outliner on once I'm done...

paquette updated this revision to Diff 153537.Jun 29 2018, 10:48 AM

Updated the patch so that it relies on the changes in D48776. That patch provides the scaffolding for allowing the outliner to be turned on by default by some target. This one flips the switch on AArch64 for -Oz.

efriedma accepted this revision.Jul 26 2018, 5:23 PM

LGTM.

test/CodeGen/AArch64/O3-pipeline.ll
158

I'm a little concerned about the impact of this change on peak memory usage, but hopefully it isn't a big deal in practice.

This revision is now accepted and ready to land.Jul 26 2018, 5:23 PM
paquette closed this revision.Jul 27 2018, 9:45 AM

Thanks!

Committed in r338133. (https://reviews.llvm.org/rL338133)