[AMDGPU] Allow index optimisation in SIPreEmitPeephole for bundles

[critson]

Summary

Add code so duplication index register changes can be removed from
inside bundles.

[foad]

Add code so duplication mode register changes

"duplicated"?

Needs a test. Also can you explain what bundles these are and why it's OK to modify them? In general I think we should err on the side of not modifying bundles, otherwise what's the point of bundling them?

[critson]

In D98940#2637112, @foad wrote:

Add code so duplication mode register changes

"duplicated"?

Needs a test. Also can you explain what bundles these are and why it's OK to modify them? In general I think we should err on the side of not modifying bundles, otherwise what's the point of bundling them?

GPR index register settings are bundled with their associated instructions:
e.g.

BUNDLE ... {
    S_SET_GPR_IDX_ON $sgpr2, 1, implicit-def $m0, implicit-def undef $mode, implicit $m0, implicit $mode
    $vgpr0 = V_MOV_B32_e32 undef $vgpr2, ...
    S_SET_GPR_IDX_OFF implicit-def $mode, implicit internal $mode
  }

Currently these are unbundled in SIMemoryLegalizer.
Code in SIPreEmitPeephole looks for patterns of duplication changes, e.g.:

set value
op1
set off
set value
op2
set off

reduces these to:

set value
op1
op2
set off

If we plan to stop unbundling everything in SIMemoryLegalizer then this change is required for the optimisation to work.

[foad]

In D98940#2637132, @critson wrote:

In D98940#2637112, @foad wrote:

Also can you explain what bundles these are and why it's OK to modify them? In general I think we should err on the side of not modifying bundles, otherwise what's the point of bundling them?

GPR index register settings are bundled with their associated instructions:
e.g.

BUNDLE ... {
    S_SET_GPR_IDX_ON $sgpr2, 1, implicit-def $m0, implicit-def undef $mode, implicit $m0, implicit $mode
    $vgpr0 = V_MOV_B32_e32 undef $vgpr2, ...
    S_SET_GPR_IDX_OFF implicit-def $mode, implicit internal $mode
  }

Currently these are unbundled in SIMemoryLegalizer.
Code in SIPreEmitPeephole looks for patterns of duplication changes, e.g.:

set value
op1
set off
set value
op2
set off

reduces these to:

set value
op1
op2
set off

OK, thanks, that does sound pretty specific to the exact kind of bundle, so I guess it's OK.

If we plan to stop unbundling everything in SIMemoryLegalizer then this change is required for the optimisation to work.

OK, but really this change should either have its own test, or it should be combined with the change to SIMemoryLegalizer.

llvm/lib/Target/AMDGPU/SIPreEmitPeephole.cpp

321

Maybe worth a comment why we are deliberately doing this inside bundles?

[critson]

• Add basic tests (copies of unbundled ones)
• Add comment w.r.t. need to process bundles

[critson]

In D98940#2637166, @foad wrote:

OK, but really this change should either have its own test, or it should be combined with the change to SIMemoryLegalizer.

I am not against combining this with that change; however, I am still testing that change in case it throws up issues.
Combined with this, the net effect of both changes on lit tests is a single instruction moving (because bundles changed the insertion point of a s_waitcnt).

[foad]

Thanks!

[t-tye]

In D98940#2637166, @foad wrote:

Currently these are unbundled in SIMemoryLegalizer.

Why is the SIMemoryLegalizer the right place to do the unbundling? Why would the memory SIMemoryLegalizer know that it has "permission" to do that? The SIMemoryLegalizer was intended to do the single job of expanding the atomics semantics.

[foad]

In D98940#2637384, @t-tye wrote:

In D98940#2637166, @foad wrote:

Currently these are unbundled in SIMemoryLegalizer.

Why is the SIMemoryLegalizer the right place to do the unbundling? Why would the memory SIMemoryLegalizer know that it has "permission" to do that? The SIMemoryLegalizer was intended to do the single job of expanding the atomics semantics.

See D72737 and 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.

[t-tye]

In D98940#2637391, @foad wrote:

In D98940#2637384, @t-tye wrote:

In D98940#2637166, @foad wrote:

Currently these are unbundled in SIMemoryLegalizer.

Why is the SIMemoryLegalizer the right place to do the unbundling? Why would the memory SIMemoryLegalizer know that it has "permission" to do that? The SIMemoryLegalizer was intended to do the single job of expanding the atomics semantics.

See D72737 and 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?

[foad]

See D72737 and 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 added some unbundling (only of bundles containing loads or stores) and D91048 added some more (all bundles). The intention here is just to undo the D91048 part.

[t-tye]

In D98940#2637447, @foad wrote:

See D72737 and 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 added some unbundling (only of bundles containing loads or stores) and D91048 added some more (all bundles). The intention here is just to undo the 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?

[foad]

In D98940#2637450, @t-tye wrote:

In D98940#2637447, @foad wrote:

See D72737 and 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 added some unbundling (only of bundles containing loads or stores) and D91048 added some more (all bundles). The intention here is just to undo the 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.

[t-tye]

In D98940#2637456, @foad wrote:

In D98940#2637450, @t-tye wrote:

In D98940#2637447, @foad wrote:

See D72737 and 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 added some unbundling (only of bundles containing loads or stores) and D91048 added some more (all bundles). The intention here is just to undo the 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.

@rampitec what do your think?

[rampitec]

In D98940#2637503, @t-tye wrote:

In D98940#2637456, @foad wrote:

In D98940#2637450, @t-tye wrote:

In D98940#2637447, @foad wrote:

See D72737 and 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 added some unbundling (only of bundles containing loads or stores) and D91048 added some more (all bundles). The intention here is just to undo the 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.

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

[kerbowa]

It seems like there is an issue with defining exactly which bundles should be allowed to be modified and what passes should be allowed to modify them. There is already a pass that will do unbundling and that seems like the best place to do that if we enabled it.

At a base level, I think of a bundle as a sequence of instructions that shouldn't be rescheduled. Beyond that, there is a lot of ambiguity. We already have a few passes that look into bundles and modify them (hazard, waitcnt, peephole...). It seems error prone to require people writing new passes or new methods of bundling instructions to be aware of exactly whether each pass should consider bundles or how passes may potentially modify bundles.

If the direction we are moving in is to keep bundles intact and have each pass be responsible for understanding how it interacts with bundles then I think the memory legalizer should not be unbundling anything. I could work on that change if we agree.

It would help if we had more clarity on what exactly we thought a bundle should be and eventually had more granularity to identify types of bundles like Jay/Stas were suggesting.

[critson]

In D98940#2637931, @kerbowa wrote:

It would help if we had more clarity on what exactly we thought a bundle should be and eventually had more granularity to identify types of bundles like Jay/Stas were suggesting.

I agree, we should clarify current bundle usage and which bundles can/cannot be touched.

llvm/lib/Target/AMDGPU/SIPreEmitPeephole.cpp

bool SIPreEmitPeephole::optimizeSetGPR(MachineInstr &First,
   Register IdxReg = Idx->isReg() ? Idx->getReg() : Register();
   SmallVector<MachineInstr *, 4> ToRemove;
   bool IdxOn = true;
 
   if (!MI.isIdenticalTo(First))
     return false;
 
   // Scan back to find an identical S_SET_GPR_IDX_ON
-  for (MachineBasicBlock::iterator I = std::next(First.getIterator()),
-       E = MI.getIterator(); I != E; ++I) {
+  for (MachineBasicBlock::instr_iterator I = std::next(First.getIterator()),
+                                         E = MI.getIterator();
+       I != E; ++I) {
+    if (I->isBundle())
+      continue;
     switch (I->getOpcode()) {
     case AMDGPU::S_SET_GPR_IDX_MODE:
       return false;
     case AMDGPU::S_SET_GPR_IDX_OFF:
       IdxOn = false;
       ToRemove.push_back(&*I);
       break;
     default:
default:
             !((I->getOpcode() == AMDGPU::V_MOV_B32_e32 &&
                I->hasRegisterImplicitUseOperand(AMDGPU::M0)) ||
               I->getOpcode() == AMDGPU::V_MOV_B32_indirect))
           return false;
       }
     }
   }
 
-  MI.eraseFromParent();
+  MI.eraseFromBundle();
   for (MachineInstr *RI : ToRemove)
-    RI->eraseFromParent();
+    RI->eraseFromBundle();
   return true;
 }
 
 bool SIPreEmitPeephole::runOnMachineFunction(MachineFunction &MF) {
   const GCNSubtarget &ST = MF.getSubtarget<GCNSubtarget>();
   TII = ST.getInstrInfo();
   TRI = &TII->getRegisterInfo();
   MachineBasicBlock *EmptyMBBAtEnd = nullptr;
if (!ST.hasVGPRIndexMode())
       continue;
 
     MachineInstr *SetGPRMI = nullptr;
     const unsigned Threshold = 20;
     unsigned Count = 0;
     // Scan the block for two S_SET_GPR_IDX_ON instructions to see if a
     // second is not needed. Do expensive checks in the optimizeSetGPR()
     // and limit the distance to 20 instructions for compile time purposes.
-    for (MachineBasicBlock::iterator MBBI = MBB.begin(); MBBI != MBBE; ) {
+    for (MachineBasicBlock::instr_iterator MBBI = MBB.instr_begin();
+         MBBI != MBBE;) {
       MachineInstr &MI = *MBBI;
       ++MBBI;
 
       if (Count == Threshold)
         SetGPRMI = nullptr;
       else
         ++Count;