[PATCH] D101468: [Passes] Run sinking/hoisting in SimplifyCFG earlier.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

In D101468#2723323 <https://reviews.llvm.org/D101468#2723323>, @lebedev.ri wrote:

> In D101468#2723273 <https://reviews.llvm.org/D101468#2723273>, @nikic wrote:
>
>> In D101468#2723229 <https://reviews.llvm.org/D101468#2723229>, @lebedev.ri wrote:
>>
>>> In D101468#2723180 <https://reviews.llvm.org/D101468#2723180>, @nikic wrote:
>>>
>>>> Thanks! I like this a lot more than the LoopVectorize variant. I also think it makes sense that this happens at the end of the "function simplification" pipeline, which means that the inliner will see the hoisted/sunk IR, which should result in a more accurate cost (as sinkable/hoistable instructions will not be counted multiple times).
>>>
>>> Nonono, this is actually the opposite from what should happen.
>>> We will then over-inline, and grow the size even more.
>>
>> I don't follow. It makes the inlining cost more accurate, so if that results in over-inlining, then the issue would be with the inlining cost model, not this change. It is my understanding that the inliner should see functions in their maximally simplified form (and prior to size-increasing optimizations like runtime unrolling and vectorization), so that it can make the most accurate decisions. Intentionally crippling the function simplification pipeline to get less inlining seems rather backward. Taken ad absurdum, that would mean that we shouldn't simplify functions prior to inlining at all.
>>
>>> This is even visible in the http://llvm-compile-time-tracker.com/compare.php?from=2ea7fb7b1c045a7d60fcccf3df3ebb26aa3699e5&to=e58b4a763c691da651f25996aad619cb3d946faf&stat=size-total
>>
>> This looks pretty normal for a phase ordering change. SPASS on `O3` is up 0.46%, lencod on `ThinLTO` is down 0.66%. The geomeans are 0.1% up or down for NewPM.
>
> I see. So i take it, D101231 <https://reviews.llvm.org/D101231>, if restricted to function-terminating block, makes more sense to you now, in general?

I think I might be missing how D101231 <https://reviews.llvm.org/D101231> is related to the current patch.

Is the concern that we may hoist/sink instructions from cold into hot blocks? I think for hosting this should definitely not happen, because we only hoist common instructions on both paths. I think the same is mostly true for sinking, although it supports sinking from only a subset of predecessors I think.

In any case, IIUC the size reduction should be accurate and I fail to see how this would lead to over-inlining. If I am missing anything, it would be great if you could elaborate in a bit more detail what cases you are concerned about.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101468