[PATCH] D94232: [LoopRotate] Add PrepareForLTO stage, avoid rotating with inline cands (WIP).

Mon Jan 18 02:09:52 PST 2021

sanwou01 added a comment.

In D94232#2503921 <https://reviews.llvm.org/D94232#2503921>, @fhahn wrote:

> In D94232#2500595 <https://reviews.llvm.org/D94232#2500595>, @sanwou01 wrote:
>
>> [...]
>
> Yes, I think the only thing outstanding with respect to testing is the `omnetpp_r` failure. Can you confirm if that is caused by the patch or not?
>
> And any review of the patch would be appreciated of course!

(I thought I posted this on Friday, but looks like I forgot). The omnetpp_r crash on SPEC 2017 has not been seen again in subsequent runs so this might have been a fluke. Performance on the new runs looks fine, no change on omnetpp_r.

In term of review, could you add a test case? A `-prepare-for-lto` flag for loop rotate for testing purposes seems reasonable to me.

In D94232#2503930 <https://reviews.llvm.org/D94232#2503930>, @xbolva00 wrote:

> In D94232#2498309 <https://reviews.llvm.org/D94232#2498309>, @sanwou01 wrote:
>
>> 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.
>
> This is worrying, -g3 should not cause miscompiles w/o this patch.

I agree, but I don't think it should block the patch going in. I'm trying to reduce it on one of the benchmark. It only shows up with LTO enabled, so it is quite slow going. (I'm having a look at the LLVM test suite as well to see if it shows up at all there, hopefully in a slightly smaller form.)

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94232/new/

https://reviews.llvm.org/D94232