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 ↗

(On Diff #116708)

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 ↗

(On Diff #117363)

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 ↗

(On Diff #117363)

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

1 ↗

(On Diff #117363)

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

3 ↗

(On Diff #117363)

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 ↗

(On Diff #117416)

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

156 ↗

(On Diff #117416)

Post increment is broken here.

157 ↗

(On Diff #117416)

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

175 ↗

(On Diff #117416)

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

test/CodeGen/AMDGPU/readlane_exec0.mir

3 ↗

(On Diff #117416)

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

llvm/trunk/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();

llvm/trunk/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
+...