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

[Jay Foad via Phabricator via llvm-commits]

foad added a comment.

In D98940#2637132 <https://reviews.llvm.org/D98940#2637132>, @critson wrote:

> In D98940#2637112 <https://reviews.llvm.org/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.



================
Comment at: llvm/lib/Target/AMDGPU/SIPreEmitPeephole.cpp:321
     // 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;) {
----------------
Maybe worth a comment why we are deliberately doing this inside bundles?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98940