[AMDGPU] Skip CFIInstructions in SIInsertWaitcnts

[scott.linder]

Summary

CFI emitted during PEI at the beginning of the prologue needs to apply
to any inserted waitcnts on function entry.

[scott.linder]

git clang-format

[arsenm]

Needs test

[scott.linder]

Add a test

[arsenm]

llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp

1644

Why just CFI? Why not isMetaInstruction?

llvm/test/CodeGen/AMDGPU/waitcnt-skip-cfi.mir

3–9 ↗

(On Diff #252965)

Shouldn't need IR section

[scott.linder]

llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp

1644

Seems reasonable to me. Is there a reason why isMetaInstruction doesn't include PHIs?

[arsenm]

llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp

1644

It's a real instruction that will emit a copy

[scott.linder]

Skip all meta instructions, not just CFI

[scott.linder]

I may have pushed this up to soon, I missed some tests, and after thinking it
through again I don't know if it is actually correct to put the DEBUG_VALUE
before the waitcnt if the waitcnt could be ensuring some writes are available?

[arsenm]

dbg.value isn't supposed to change codegen, so always ignoring them seems like the correct choice?

[scott.linder]

Right, but the placement of the meta instructions here can't affect change codegen, can it? Also, if we do need to skip the DEBUG_VALUEs it may also be that we would need to either update or drop them to avoid producing incorrect debug info. I'm not actually certain this is an issue though, but the situation I have in mind is if we pass an argument by-ref or on the stack, and in the callee we claim the value is in memory when the appropriate counts haven't been waited on yet.

[arsenm]

In D76881#1946435, @scott.linder wrote:

Right, but the placement of the meta instructions here can't affect change codegen, can it? Also, if we do need to skip the DEBUG_VALUEs it may also be that we would need to either update or drop them to avoid producing incorrect debug info. I'm not actually certain this is an issue though, but the situation I have in mind is if we pass an argument by-ref or on the stack, and in the callee we claim the value is in memory when the appropriate counts haven't been waited on yet.

Doesn't the placement of CFI matter relative to the real instructions?

[scott.linder]

In D76881#1946539, @arsenm wrote:

In D76881#1946435, @scott.linder wrote:

Right, but the placement of the meta instructions here can't affect change codegen, can it? Also, if we do need to skip the DEBUG_VALUEs it may also be that we would need to either update or drop them to avoid producing incorrect debug info. I'm not actually certain this is an issue though, but the situation I have in mind is if we pass an argument by-ref or on the stack, and in the callee we claim the value is in memory when the appropriate counts haven't been waited on yet.

Doesn't the placement of CFI matter relative to the real instructions?

Yes, and same for DEBUG_VALUE, which is where my uncertainty comes up. For CFI we absolutely need the directives at the beginning of the block to precede any real instructions, because they apply "on entry". For example, we reference the ABI return address registers here, and we can't have these be defined incorrectly until after the initial s_waitcnt. Conversely for DEBUG_VALUEs it may be that we are constructing them assuming they can reference memory locations for arguments that are only valid after the initial s_waitcnt. So I think we need to handle the two cases distinctly.

[arsenm]

In D76881#1946554, @scott.linder wrote:

In D76881#1946539, @arsenm wrote:

In D76881#1946435, @scott.linder wrote:

Right, but the placement of the meta instructions here can't affect change codegen, can it? Also, if we do need to skip the DEBUG_VALUEs it may also be that we would need to either update or drop them to avoid producing incorrect debug info. I'm not actually certain this is an issue though, but the situation I have in mind is if we pass an argument by-ref or on the stack, and in the callee we claim the value is in memory when the appropriate counts haven't been waited on yet.

Doesn't the placement of CFI matter relative to the real instructions?

Yes, and same for DEBUG_VALUE, which is where my uncertainty comes up. For CFI we absolutely need the directives at the beginning of the block to precede any real instructions, because they apply "on entry". For example, we reference the ABI return address registers here, and we can't have these be defined incorrectly until after the initial s_waitcnt. Conversely for DEBUG_VALUEs it may be that we are constructing them assuming they can reference memory locations for arguments that are only valid after the initial s_waitcnt. So I think we need to handle the two cases distinctly.

ok

[t-tye]

In D76881#1946594, @arsenm wrote:

In D76881#1946554, @scott.linder wrote:

In D76881#1946539, @arsenm wrote:

In D76881#1946435, @scott.linder wrote:

Right, but the placement of the meta instructions here can't affect change codegen, can it? Also, if we do need to skip the DEBUG_VALUEs it may also be that we would need to either update or drop them to avoid producing incorrect debug info. I'm not actually certain this is an issue though, but the situation I have in mind is if we pass an argument by-ref or on the stack, and in the callee we claim the value is in memory when the appropriate counts haven't been waited on yet.

Doesn't the placement of CFI matter relative to the real instructions?

Yes, and same for DEBUG_VALUE, which is where my uncertainty comes up. For CFI we absolutely need the directives at the beginning of the block to precede any real instructions, because they apply "on entry". For example, we reference the ABI return address registers here, and we can't have these be defined incorrectly until after the initial s_waitcnt. Conversely for DEBUG_VALUEs it may be that we are constructing them assuming they can reference memory locations for arguments that are only valid after the initial s_waitcnt. So I think we need to handle the two cases distinctly.

ok

From a debug information point of view, the memory is updated when the instruction is issued, regardless of the waitcnt having completed. The Debugger has to stop the wave before it can see that waves state. The act of stopping a wave ensures that all memory actions it has performed are made coherent with the debugger. That includes ensuring all outstanding operations are complete, and caches are flushed and written back.

[scott.linder]

In D76881#1946979, @t-tye wrote:

In D76881#1946594, @arsenm wrote:

In D76881#1946554, @scott.linder wrote:

In D76881#1946539, @arsenm wrote:

In D76881#1946435, @scott.linder wrote:

Right, but the placement of the meta instructions here can't affect change codegen, can it? Also, if we do need to skip the DEBUG_VALUEs it may also be that we would need to either update or drop them to avoid producing incorrect debug info. I'm not actually certain this is an issue though, but the situation I have in mind is if we pass an argument by-ref or on the stack, and in the callee we claim the value is in memory when the appropriate counts haven't been waited on yet.

Doesn't the placement of CFI matter relative to the real instructions?

Yes, and same for DEBUG_VALUE, which is where my uncertainty comes up. For CFI we absolutely need the directives at the beginning of the block to precede any real instructions, because they apply "on entry". For example, we reference the ABI return address registers here, and we can't have these be defined incorrectly until after the initial s_waitcnt. Conversely for DEBUG_VALUEs it may be that we are constructing them assuming they can reference memory locations for arguments that are only valid after the initial s_waitcnt. So I think we need to handle the two cases distinctly.

ok

From a debug information point of view, the memory is updated when the instruction is issued, regardless of the waitcnt having completed. The Debugger has to stop the wave before it can see that waves state. The act of stopping a wave ensures that all memory actions it has performed are made coherent with the debugger. That includes ensuring all outstanding operations are complete, and caches are flushed and written back.

Ok, that makes sense, in that case I do think that the patch is correct as-is, skipping all meta instructions.

[RamNalamothu]

Rebase

llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp

bool SIInsertWaitcnts::runOnMachineFunction(MachineFunction &MF) {
   if (!MFI->isEntryFunction()) {
     // Wait for any outstanding memory operations that the input registers may
     // depend on. We can't track them and it's better to the wait after the
     // costly call sequence.
 
     // TODO: Could insert earlier and schedule more liberally with operations
     // that only use caller preserved registers.
     MachineBasicBlock &EntryBB = MF.front();
+    MachineBasicBlock::iterator I = EntryBB.begin();
+    for (MachineBasicBlock::iterator E = EntryBB.end();
+         I != E && (I->isPHI() || I->isCFIInstruction()); ++I)
+      ;
+    BuildMI(EntryBB, I, DebugLoc(), TII->get(AMDGPU::S_WAITCNT))
+      .addImm(0);
     if (ST->hasVscnt())
-      BuildMI(EntryBB, EntryBB.getFirstNonPHI(), DebugLoc(),
+      BuildMI(EntryBB, I, DebugLoc(),
               TII->get(AMDGPU::S_WAITCNT_VSCNT))
       .addReg(AMDGPU::SGPR_NULL, RegState::Undef)
       .addImm(0);
-    BuildMI(EntryBB, EntryBB.getFirstNonPHI(), DebugLoc(), TII->get(AMDGPU::S_WAITCNT))
-      .addImm(0);
 
     Modified = true;
   }
 
   return Modified;
 }