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

[Florian Hahn via Phabricator via llvm-commits]

fhahn updated this revision to Diff 317374.
fhahn added a comment.
Herald added a subscriber: steven_wu.

In D94232#2504325 <https://reviews.llvm.org/D94232#2504325>, @sanwou01 wrote:

> 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.

Sounds good, I added a new `-rotation-prepare-for-lto` option and a test using it.

> 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.)

I cannot reproduce this with regular LTO, as I mentioned earlier I do not think this is caused by the patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94232

Files:
  llvm/include/llvm/Analysis/CodeMetrics.h
  llvm/include/llvm/Transforms/Scalar.h
  llvm/include/llvm/Transforms/Scalar/LoopRotation.h
  llvm/include/llvm/Transforms/Utils/LoopRotationUtils.h
  llvm/lib/Analysis/CodeMetrics.cpp
  llvm/lib/Passes/PassBuilder.cpp
  llvm/lib/Transforms/IPO/PassManagerBuilder.cpp
  llvm/lib/Transforms/Scalar/LoopRotation.cpp
  llvm/lib/Transforms/Utils/LoopRotationUtils.cpp
  llvm/test/Transforms/LoopRotate/call-prepare-for-lto.ll

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D94232.317374.patch
Type: text/x-patch
Size: 12532 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210118/fe542aab/attachment-0001.bin>