AMDGPU: Fix missing read/writelane cases to skip with exec=0

[arsenm]

Summary

Writelane was missing. Additionally, since we emit the final encoded
instructions, this wasn't really matching readlane either.

[nhaehnle]

Adding writelane absolutely makes sense to me, but:

Additionally, since we emit the final encoded instructions, this wasn't really matching readlane either.

:(

We have a bunch of places where we _always_ use real instructions, and presumably those are fine. But if we let real instructions creep into places where we usually use pseudos, we end up making the backend less efficient because over time, everybody will have to check for pseudos and reals (like in this particular case here). Can we instead add some sort of check that those real instructions aren't used during CodeGen? (The MI verifier should be able to check for this, for example.)

[arsenm]

In D89610#2339205, @nhaehnle wrote:

Adding writelane absolutely makes sense to me, but:

Additionally, since we emit the final encoded instructions, this wasn't really matching readlane either.

:(

We have a bunch of places where we _always_ use real instructions, and presumably those are fine. But if we let real instructions creep into places where we usually use pseudos, we end up making the backend less efficient because over time, everybody will have to check for pseudos and reals (like in this particular case here). Can we instead add some sort of check that those real instructions aren't used during CodeGen? (The MI verifier should be able to check for this, for example.)

There is some reason we use the encoded versions for readlane/writelane that I never remember what it is

[nhaehnle]

In D89610#2339215, @arsenm wrote:

In D89610#2339205, @nhaehnle wrote:

Adding writelane absolutely makes sense to me, but:

Additionally, since we emit the final encoded instructions, this wasn't really matching readlane either.

:(

We have a bunch of places where we _always_ use real instructions, and presumably those are fine. But if we let real instructions creep into places where we usually use pseudos, we end up making the backend less efficient because over time, everybody will have to check for pseudos and reals (like in this particular case here). Can we instead add some sort of check that those real instructions aren't used during CodeGen? (The MI verifier should be able to check for this, for example.)

There is some reason we use the encoded versions for readlane/writelane that I never remember what it is

Would be good to understand that? A quick hack to replace the use of TII->getMCOpcodeFromPseudo from SIRegisterInfo.cpp shows a whole bunch of lit test changes, but so far the ones I've looked at are just scheduling changes that ought to be benign.

[foad]

In D89610#2339342, @nhaehnle wrote:

In D89610#2339215, @arsenm wrote:

In D89610#2339205, @nhaehnle wrote:

Adding writelane absolutely makes sense to me, but:

Additionally, since we emit the final encoded instructions, this wasn't really matching readlane either.

:(

We have a bunch of places where we _always_ use real instructions, and presumably those are fine. But if we let real instructions creep into places where we usually use pseudos, we end up making the backend less efficient because over time, everybody will have to check for pseudos and reals (like in this particular case here). Can we instead add some sort of check that those real instructions aren't used during CodeGen? (The MI verifier should be able to check for this, for example.)

There is some reason we use the encoded versions for readlane/writelane that I never remember what it is

Would be good to understand that? A quick hack to replace the use of TII->getMCOpcodeFromPseudo from SIRegisterInfo.cpp shows a whole bunch of lit test changes, but so far the ones I've looked at are just scheduling changes that ought to be benign.

rG3db6ba8cfae254ecd3b5a8166d857cadb19d04e4.

[foad]

In D89610#2339342, @nhaehnle wrote:

In D89610#2339215, @arsenm wrote:

In D89610#2339205, @nhaehnle wrote:

Adding writelane absolutely makes sense to me, but:

Additionally, since we emit the final encoded instructions, this wasn't really matching readlane either.

:(

We have a bunch of places where we _always_ use real instructions, and presumably those are fine. But if we let real instructions creep into places where we usually use pseudos, we end up making the backend less efficient because over time, everybody will have to check for pseudos and reals (like in this particular case here). Can we instead add some sort of check that those real instructions aren't used during CodeGen? (The MI verifier should be able to check for this, for example.)

There is some reason we use the encoded versions for readlane/writelane that I never remember what it is

Would be good to understand that? A quick hack to replace the use of TII->getMCOpcodeFromPseudo from SIRegisterInfo.cpp shows a whole bunch of lit test changes, but so far the ones I've looked at are just scheduling changes that ought to be benign.

I've cooked this up into D90401.

[arsenm]

Don't bother with encoded versions

[foad]

LGTM.

[arsenm]

790f5771fd00e18e7eae7f9649ab529c20dd7823

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

bool SIInstrInfo::hasUnwantedEffectsWhenEXECEmpty(const MachineInstr &MI) const {
   if (modifiesModeRegister(MI))
     return true;
 
   // These are like SALU instructions in terms of effects, so it's questionable
   // whether we should return true for those.
   //
   // However, executing them with EXEC = 0 causes them to operate on undefined
   // data, which we avoid by returning true here.
-  if (Opcode == AMDGPU::V_READFIRSTLANE_B32 || Opcode == AMDGPU::V_READLANE_B32)
+  if (Opcode == AMDGPU::V_READFIRSTLANE_B32 ||
+      Opcode == AMDGPU::V_READLANE_B32 || Opcode == AMDGPU::V_WRITELANE_B32)
     return true;
 
   return false;
 }
 
 bool SIInstrInfo::mayReadEXEC(const MachineRegisterInfo &MRI,
                               const MachineInstr &MI) const {
   if (MI.isMetaInstruction())

llvm/test/CodeGen/AMDGPU/remove-short-exec-branches-special-instructions.mir

body: |
 
   bb.1:
     $vgpr0 = V_MOV_B32_e32 0, implicit $exec
     S_ROUND_MODE 3, implicit-def $mode, implicit $mode
 
   bb.2:
     S_ENDPGM 0
 ...
+
+---
+
+name: need_skip_writelane_b32
+body: |
+  ; CHECK-LABEL: name: need_skip_writelane_b32
+  ; CHECK: bb.0:
+  ; CHECK:   successors: %bb.1(0x40000000), %bb.2(0x40000000)
+  ; CHECK:   S_CBRANCH_EXECZ %bb.2, implicit $exec
+  ; CHECK: bb.1:
+  ; CHECK:   successors: %bb.2(0x80000000)
+  ; CHECK:   $sgpr0 = IMPLICIT_DEF
+  ; CHECK:   $vgpr0 = V_WRITELANE_B32 $sgpr0, 0, $vgpr0
+  ; CHECK: bb.2:
+  ; CHECK:   S_ENDPGM 0
+  bb.0:
+    successors: %bb.1, %bb.2
+    S_CBRANCH_EXECZ %bb.2, implicit $exec
+
+  bb.1:
+    successors: %bb.2
+    $sgpr0 = IMPLICIT_DEF
+    $vgpr0 = V_WRITELANE_B32 $sgpr0, 0, $vgpr0
+
+  bb.2:
+    S_ENDPGM 0
+...
+
+---
+name: need_skip_readlane_b32
+body: |
+  ; CHECK-LABEL: name: need_skip_readlane_b32
+  ; CHECK: bb.0:
+  ; CHECK:   successors: %bb.1(0x40000000), %bb.2(0x40000000)
+  ; CHECK:   S_CBRANCH_EXECZ %bb.2, implicit $exec
+  ; CHECK: bb.1:
+  ; CHECK:   successors: %bb.2(0x80000000)
+  ; CHECK:   $vgpr0 = IMPLICIT_DEF
+  ; CHECK:   $sgpr0 = V_READLANE_B32 $vgpr0, 0
+  ; CHECK: bb.2:
+  ; CHECK:   S_ENDPGM 0
+  bb.0:
+    successors: %bb.1, %bb.2
+    S_CBRANCH_EXECZ %bb.2, implicit $exec
+
+  bb.1:
+    successors: %bb.2
+    $vgpr0 = IMPLICIT_DEF
+    $sgpr0 = V_READLANE_B32 $vgpr0, 0
+
+  bb.2:
+    S_ENDPGM 0
+...