Page MenuHomePhabricator

[LoopRotate] Add PrepareForLTO stage, avoid rotating with inline cands (WIP).
Needs ReviewPublic

Authored by fhahn on Thu, Jan 7, 6:14 AM.

Details

Summary

WIP because it is still a bit rough around the edges.

D84108 exposed a bad interaction between inlining and loop-rotation
during regular LTO, which is causing notable regressions in at least
CINT2006/473.astar.

The problem boils down to: we now rotate a loop just before the vectorizer
which requires duplicating a function call in the preheader when compiling
the individual files ('prepare for LTO'). But this then prevents further
inlining of the function during LTO.

This patch tries to resolve this issue by making LoopRotate more
conservative with respect to rotating loops that have inline-able calls
during the 'prepare for LTO' stage.

I think this change intuitively improves the current situation in
general. Loop-rotate tries hard to avoid creating headers that are 'too
big'. At the moment, it assumes all inlining already happened and the
cost of duplicating a call is equal to just doing the call. But with LTO,
inlining also happens during full LTO and it is possible that a previously
duplicated call is actually a huge function which gets inlined
during LTO.

From the perspective of LV, not much should change overall. Most loops
calling user-provided functions won't get vectorized to start with
(unless we can infer that the function does not touch memory, has no
other side effects). If we do not inline the 'inline-able' call during
the LTO stage, we merely delayed loop-rotation & vectorization. If we
inline during LTO, chances should be very high that the inlined code is
itself vectorizable or the user call was not vectorizable to start with.

There could of course be scenarios where we inline a sufficiently large
function with code not profitable to vectorize, which would have be
vectorized earlier (by scalarzing the call). But even in that case,
there probably is no big performance impact, because it should be mostly
down to the cost-model to reject vectorization in that case. And then
the version with scalarized calls should also not be beneficial. In a way,
LV should have strictly more information after inlining and make more
accurate decisions (barring cost-model issues).

There is of course plenty of room for things to go wrong unexpectedly,
so we need to keep a close look at actual performance and address any
follow-up issues.

I took a look at the impact on statistics for
MultiSource/SPEC2000/SPEC2006. There are a few benchmarks with fewer
loops rotated, but no change to the number of loops vectorized.

I still need to think on how to best test the change. Perhaps add a
-prepare-for-lto flag to LoopRotate?

Diff Detail

Event Timeline

fhahn created this revision.Thu, Jan 7, 6:14 AM
fhahn requested review of this revision.Thu, Jan 7, 6:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptThu, Jan 7, 6:14 AM

Thanks, Florian. I'll give the patch a run through our benchmarking.

it assumes all inlining already happened and the cost of duplicating a call is equal to just doing the call

This was the bit of information I was missing, thanks! With that, I think your proposed change makes a lot of sense.

@fhahn I'm afraid I'm seeing runtime and verification errors on perlbench, gcc, gobmk, and astar in SPEC INT 2006. I'm guessing something later in the pipe really doesn't like it when certain loops aren't rotated?

fhahn updated this revision to Diff 315338.Fri, Jan 8, 3:46 AM

@fhahn I'm afraid I'm seeing runtime and verification errors on perlbench, gcc, gobmk, and astar in SPEC INT 2006. I'm guessing something later in the pipe really doesn't like it when certain loops aren't rotated?

Hmmm, I think it would be very surprising, if not rotating some loops would expose any serious mis-compiles in SPEC. When building with -Oz for example, hardly any loops are rotated.

What configuration are you using? I couldn't reproduce any SPEC2006 INT failures on either x86/ARM64 with -O3 -flto.

Our flags are -mcpu=native -O3 -fomit-frame-pointer -flto on a Neoverse-N1 based system, so -mcpu=neoverse-n1 should do the same thing.

It looks like this might be an issue on our end, possibly unrelated to the patch.

Yep, definitely a problem on our end; I've got similar symptoms in other runs. Sorry about the noise, and I will try to get some perf data once we sort this out.

Turns out the failing benchmarks are due to a miscompilation with -g3 (which we add to profiled runs). The patch does seem to make that miscompilation more likely. I'll try to reduce that separately, but at least I'll have some performance numbers shortly.

So on SPEC 2006 this fixes astar (+9.6%) as well as shakes things up enough to "fix" h264ref (+9.0%, see D93946). Other notable changes are libquantum (+3.2%) and omnetpp (-2.8%). Geomean is +1.5%.

On SPEC 2017 there is still a problem with omnetpp_r (runtime error), which I'll have a look at next, but is otherwise neutral on geomean with only minor wobbles (+1%, -0.5% at most) on individual benchmarks.

With this having a substantial impact on performance, I'm quite keen to help get this in soon, ideally before the LLVM 12 branch, so please let me know if there is anything else I can do to help.

So on SPEC 2006 this fixes astar (+9.6%) as well as shakes things up enough to "fix" h264ref (+9.0%, see D93946). Other notable changes are libquantum (+3.2%) and omnetpp (-2.8%). Geomean is +1.5%.

Thank you very much for running the numbers!

Just to double check, positive here means good (I assume increase in score)?

On SPEC 2017 there is still a problem with omnetpp_r (runtime error), which I'll have a look at next, but is otherwise neutral on geomean with only minor wobbles (+1%, -0.5% at most) on individual benchmarks.

With this having a substantial impact on performance, I'm quite keen to help get this in soon, ideally before the LLVM 12 branch, so please let me know if there is anything else I can do to help.

Sounds good! I think the theory behind the change should mean it should be quite safe and the data so far seems encouraging; I can confirm similar improvements for astar on our tracking.

It would probably good to get this in well ahead of the 12.0 branch, so it has a chance to make it through various downstream perf-tracking systems.

So on SPEC 2006 this fixes astar (+9.6%) as well as shakes things up enough to "fix" h264ref (+9.0%, see D93946). Other notable changes are libquantum (+3.2%) and omnetpp (-2.8%). Geomean is +1.5%.

Thank you very much for running the numbers!

Just to double check, positive here means good (I assume increase in score)?

Yes, positive is good :)

On SPEC 2017 there is still a problem with omnetpp_r (runtime error), which I'll have a look at next, but is otherwise neutral on geomean with only minor wobbles (+1%, -0.5% at most) on individual benchmarks.

With this having a substantial impact on performance, I'm quite keen to help get this in soon, ideally before the LLVM 12 branch, so please let me know if there is anything else I can do to help.

Sounds good! I think the theory behind the change should mean it should be quite safe and the data so far seems encouraging; I can confirm similar improvements for astar on our tracking.

It would probably good to get this in well ahead of the 12.0 branch, so it has a chance to make it through various downstream perf-tracking systems.

That's great! Agreed that this could do with a little soak on main before 12 branches, which IIRC is scheduled for 26 January?