[PATCH] D98940: [AMDGPU] Allow index optimisation in SIPreEmitPeephole for bundles

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec added a comment.

In D98940#2637503 <https://reviews.llvm.org/D98940#2637503>, @t-tye wrote:

> In D98940#2637456 <https://reviews.llvm.org/D98940#2637456>, @foad wrote:
>
>> In D98940#2637450 <https://reviews.llvm.org/D98940#2637450>, @t-tye wrote:
>>
>>> In D98940#2637447 <https://reviews.llvm.org/D98940#2637447>, @foad wrote:
>>>
>>>>>> See D72737 <https://reviews.llvm.org/D72737> and D91048 <https://reviews.llvm.org/D91048> for the history of that. I'm not saying it's the right place to do it. The point of the current patch is to help us move away from that, i.e. to do less unbundling in SIMemoryLegalizer.
>>>>>
>>>>> I do not think SIMemoryLegalizer should be doing any unbundling as it is conflating tasks. So will this direction manage to eliminate it?
>>>>
>>>> No, not entirely. D72737 <https://reviews.llvm.org/D72737> added some unbundling (only of bundles containing loads or stores) and D91048 <https://reviews.llvm.org/D91048> added some more (all bundles). The intention here is just to undo the D91048 <https://reviews.llvm.org/D91048> part.
>>>
>>> Why would SIMemoryLegalizer know it is correct to do the unbundling? Presumably the instructions are in a bundle for a reason? How does SIMemoryLegalizer know that that reason is no longer necessary? Is SIMemoryLegalizer meant to know the passes hat run after it and what their needs are? Is there a point in the compilation where the need for these bundles to exist disappears? If so shouldn't that be the point the unbundling is done? It feels uncomfortable to put this kind of things in passes as it makes things very brittle. If someone made changes to the bundling would they be aware that SIMemoryLegalizer is doing this?
>>
>> Don't ask me! I'm just trying to explain the status quo. I am also uncomfortable with the idea of bundles being unbundled or modified in general, though I can see that it would be useful to be able to bundle some stuff in one pass and then later unbundle "my" bundles only -- but at the moment there is no robust way of distinguishing "my" bundles from anyone else's.
>>
>> Anyway I think you need to take this up with @rampitec who wrote D72737 <https://reviews.llvm.org/D72737>.
>
> @rampitec what do your think?

Legalizer must observe all loads and stores, however it does not look inside bundles. Therefore it unbundles something which may load or store. On practice these bundles are not needed at this point too.
Then later patch has extended unbundling to every bundle, not just load/store, which we are trying to revert now.
As a separate thing legalizer may start work inside bundles and remove unbundling completely, but this is really orthogonal.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98940