Avoid predicated execution of the basic blocks containing scalar instructions

[alex-t]

Summary

V_READFIRSTLANE and V_READLANE produce scalar result from vector operand. Scalar instructions consuming the result should not be executed if exec mask is zero.

Tests passed : make llvm-check

[rampitec]

Test is needed.

lib/Target/AMDGPU/SIInsertSkips.cpp

137

The comment is misleading. Scalar instructions executed even if exec = 0 (contrarily to the comment). That is unclear if there must be a scalar instruction consuming result of readlane too, since SGPR can be an operand of a vector instruction.

[alex-t]

1. Added the code that checks that scalar register produced by V_READFIRSTLANE/V_READLANE

Is really used by some SALU instruction. This is necessary to avoid de-optimization of those cases when the scalar register is distinctly the scalar operand of vector instruction.

2. MIR test added

[rampitec]

lib/Target/AMDGPU/SIInsertSkips.cpp

145

What if an user is actually hoisted out the block? What if that is terminator uses it? In both cases users should then read undefined data.

148

What is interesting, an user can be also another v_readlane_b32's lane select operand. Subsequent v_readlane_b32 would be executed even with exec = 0 and will read an undefined data.

test/CodeGen/AMDGPU/readlane_exec0.mir

2

Please use -check-label=GCN and GCN labels instead of CHECK. This seems to be our new convention.
Also add -verify-machineinstrs.

4

Label in this case needs to be function's name. That is to add new function to the test later.
bb.0 then becomes a regular check.

[alex-t]

It is really make sense to take care of the V_READFIRSTLANE/V_READLANE destination register under exec == 0 condition in case their source VGPR is re-defined in SI_MASK_BRANCH target block. Otherwise we assume that source VGPR is defined in one of the dominating blocks and contain correct value.

[rampitec]

lib/Target/AMDGPU/SIInsertSkips.cpp

145

It is named operand "src0", using getOperand(1) is not desirable.
Also VReg is misleading, it reads like "virtual register".

156

Post increment is broken here.

157

Please follow the general and surrounding style: brace on the same line with expression.

175

That is broken. Remember that register is physical.
Not speaking that is whole function scan is expensive.

test/CodeGen/AMDGPU/readlane_exec0.mir

4

You still need to add GCN-LABEL

[rampitec]

I would just bail on any of these instructions not trying to optimize the case, just like you did in the beginning.

[arsenm]

I want to replace this pass by always inserting the branches on execz, and have a new pass which optimizes out short jumps. Would that be easier than trying to analyze this?

[alex-t]

Reverted back to the simplest approach.

[rampitec]

LGTM

[alex-t]

This comment was removed by alex-t.

[alex-t]

r314828

lib/Target/AMDGPU/SIInsertSkips.cpp

for (MachineBasicBlock::const_iterator I = MBB.begin(), E = MBB.end();
 
       // When a uniform loop is inside non-uniform control flow, the branch
       // leaving the loop might be an S_CBRANCH_VCCNZ, which is never taken
       // when EXEC = 0. We should skip the loop lest it becomes infinite.
       if (I->getOpcode() == AMDGPU::S_CBRANCH_VCCNZ ||
           I->getOpcode() == AMDGPU::S_CBRANCH_VCCZ)
         return true;
 
+      // V_READFIRSTLANE/V_READLANE destination register may be used as operand
+      // by some SALU instruction. If exec mask is zero vector instruction
+      // defining the register that is used by the scalar one is not executed
+      // and scalar instruction will operate on undefined data. For
+      // V_READFIRSTLANE/V_READLANE we should avoid predicated execution.
+      if ((I->getOpcode() == AMDGPU::V_READFIRSTLANE_B32) ||
+          (I->getOpcode() == AMDGPU::V_READLANE_B32)) {
+        return true;
+      }
+
       if (I->isInlineAsm()) {
         const MCAsmInfo *MAI = MF->getTarget().getMCAsmInfo();
         const char *AsmStr = I->getOperand(0).getSymbolName();
 
         // inlineasm length estimate is number of bytes assuming the longest
         // instruction.
         uint64_t MaxAsmSize = TII->getInlineAsmLength(AsmStr, *MAI);
         NumInstr += MaxAsmSize / MAI->getMaxInstLength();
       } else {
         ++NumInstr;
       }
 
       if (NumInstr >= SkipThreshold)
         return true;
     }
   }
 
   return false;
 }
 
 bool SIInsertSkips::skipIfDead(MachineInstr &MI, MachineBasicBlock &NextBB) {
   MachineBasicBlock &MBB = *MI.getParent();
   MachineFunction *MF = MBB.getParent();
 
   if (MF->getFunction()->getCallingConv() != CallingConv::AMDGPU_PS ||
       !shouldSkip(MBB, MBB.getParent()->back()))
     return false;
 
   MachineBasicBlock *SkipBB = insertSkipBlock(MBB, MI.getIterator());
 
   const DebugLoc &DL = MI.getDebugLoc();
 
   // If the exec mask is non-zero, skip the next two instructions
   BuildMI(&MBB, DL, TII->get(AMDGPU::S_CBRANCH_EXECNZ))
     .addMBB(&NextBB);
 
   MachineBasicBlock::iterator Insert = SkipBB->begin();
 
   // Exec mask is zero: Export to NULL target...

test/CodeGen/AMDGPU/readlane_exec0.mir

+# RUN: llc -o - %s -march=amdgcn -mcpu=fiji  -run-pass=si-insert-skips -verify-machineinstrs | FileCheck -check-prefix=GCN %s
+
+# GCN-LABEL: readlane_exec0
+# GCN: bb.0
+# GCN: S_CBRANCH_EXECZ %bb.2
+
+---
+name: readlane_exec0
+
+body:       |
+  bb.0:
+    successors: %bb.1, %bb.2
+    liveins: %vgpr1_vgpr2:0x00000001, %vgpr2_vgpr3:0x00000003
+
+    %vgpr4 = V_AND_B32_e32 1, %vgpr1, implicit %exec
+    V_CMP_EQ_U32_e32 1, killed %vgpr4, implicit-def %vcc, implicit %exec
+    %sgpr0_sgpr1 = S_AND_SAVEEXEC_B64 killed %vcc, implicit-def %exec, implicit-def %scc, implicit %exec
+    SI_MASK_BRANCH %bb.2, implicit %exec
+    S_BRANCH %bb.1
+
+  bb.1:
+    
+   %sgpr10 = V_READFIRSTLANE_B32 %vgpr2, implicit %exec
+   %sgpr11 = V_READFIRSTLANE_B32 %vgpr3, implicit %exec
+   %sgpr10 = S_LOAD_DWORD_IMM killed %sgpr10_sgpr11, 0, 0
+   S_WAITCNT 127
+   %vgpr0 = V_XOR_B32_e32 killed %sgpr10, killed %vgpr0, implicit %exec
+   
+  bb.2:
+
+    %exec = S_OR_B64 %exec, killed %sgpr0_sgpr1, implicit-def %scc
+...