[llvm][GenericUniformity] Prevent assert while calculating temporal divergence

[yassingh]

Summary

analyzeTemporalDivergence() was missing the check for always-uniform before
evaluating weather an instruction depends on a value defined in the cycle.
Fix for #60638

[foad]

llvm/include/llvm/ADT/GenericUniformityImpl.h

814

This is too complicated. Just swap lines 814 and 817.

[yassingh]

Reordered conditions as Jay mentioned

[arsenm]

llvm/lib/CodeGen/MachineUniformityAnalysis.cpp

107

Remove dead assert

llvm/test/Analysis/DivergenceAnalysis/AMDGPU/MIR/uses-value-from-cycle.mir

5

Shouldn't need IR section?

138–158

Surely you don't need all these arguments?

[sameerds]

llvm/lib/CodeGen/MachineUniformityAnalysis.cpp

108

What does it mean to continue here? Is it true that a physical register cannot be written inside the cycle? A comment explaining why will be useful.

[yassingh]

llvm/lib/CodeGen/MachineUniformityAnalysis.cpp

108

Didn't get around to putting a lot of thought into it. Physical register operands need to be handled separately, which might also include sub-cases if it's an exec mask or function parameters/return values, etc. Continue here is just a placeholder until we have a way to deal with them.

llvm/test/Analysis/DivergenceAnalysis/AMDGPU/MIR/uses-value-from-cycle.mir

5

I took the tests from #60638 (extracted the GMIR input that was fed to uniformity analysis). I wasn't able to use @llvm.amdgcn.raw.atomic.buffer.load without including the IR section. Is there a workaround?

[sameerds]

llvm/lib/CodeGen/MachineUniformityAnalysis.cpp

108

But if the exact conclusion is not known, then the correct conclusion is to be conservative. For example, it is always safer to assume that a value is divergent rather than assuming that it is uniform. In this particular case, it means that if we don't know whether the physical register was defined inside the cycle, then it is safer to assume that it was, and hence return true. Such a register will then be marked divergent by the caller.

[sameerds]

The commit description uses a number "60638" to refer to some issue, but there is no suitable context. If this is a github issue, just putting the whole URL might be better? And do note that this needs to be in the git commit, and not just the review description on this page.

[arsenm]

In D144070#4131694, @sameerds wrote:

The commit description uses a number "60638" to refer to some issue, but there is no suitable context. If this is a github issue, just putting the whole URL might be better? And do note that this needs to be in the git commit, and not just the review description on this page.

This is the form GitHub recognizes to auto-close the issue

[arsenm]

llvm/test/Analysis/DivergenceAnalysis/AMDGPU/MIR/uses-value-from-cycle.mir

5

The intrinsic declaration is broken in some way because it's having an address computed. The reference in SI_PC_ADD_REL_OFFSET indicates this wasn't recognized as an intrinsic.

[yassingh]

Review comments

[yassingh]

In D144070#4131694, @sameerds wrote:

The commit description uses a number "60638" to refer to some issue, but there is no suitable context. If this is a github issue, just putting the whole URL might be better? And do note that this needs to be in the git commit, and not just the review description on this page.

Will also append issue link to the commit message.

llvm/test/Analysis/DivergenceAnalysis/AMDGPU/MIR/uses-value-from-cycle.mir

5

I didn't realize earlier that @llvm.amdgcn.raw.atomic.buffer.load.i32 is not actually an intrinsic just looks like one, hence appears to be broken.
Considering dropping this test as it's confusing and bloated.

[arsenm]

llvm/test/Analysis/DivergenceAnalysis/AMDGPU/MIR/uses-value-from-cycle.mir

5

This must have a test, but simpler

[yassingh]

Simplified test

[sameerds]

The change itself looks okay to me. But please wait for comments on whether the tests are okay.

[foad]

LGTM.

llvm/include/llvm/ADT/GenericUniformityImpl.h

 template <typename ContextT>
 void GenericUniformityAnalysisImpl<ContextT>::analyzeTemporalDivergence(
     const InstructionT &I, const CycleT &OuterDivCycle) {
   if (isDivergent(I))
     return;
 
   LLVM_DEBUG(dbgs() << "Analyze temporal divergence: " << Context.print(&I)
                     << "\n");
-  if (!usesValueFromCycle(I, OuterDivCycle))
+  if (isAlwaysUniform(I))
     return;
 
-  if (isAlwaysUniform(I))
+  if (!usesValueFromCycle(I, OuterDivCycle))
     return;
 
   if (markDivergent(I))
     Worklist.push_back(&I);
 }
 
 // Mark all external users of values defined inside \param
 // OuterDivCycle as divergent.

llvm/lib/Analysis/UniformityAnalysis.cpp

void llvm::GenericUniformityAnalysisImpl<SSAContext>::pushUsers(
   if (Instr.isTerminator())
     return;
   pushUsers(cast<Value>(&Instr));
 }
 
 template <>
 bool llvm::GenericUniformityAnalysisImpl<SSAContext>::usesValueFromCycle(
     const Instruction &I, const Cycle &DefCycle) const {
-  if (isAlwaysUniform(I))
-    return false;
+  assert(!isAlwaysUniform(I));
   for (const Use &U : I.operands()) {
     if (auto *I = dyn_cast<Instruction>(&U)) {
       if (DefCycle.contains(I->getParent()))
         return true;
     }
   }
   return false;
 }

llvm/lib/CodeGen/MachineUniformityAnalysis.cpp

 template <>
 bool llvm::GenericUniformityAnalysisImpl<MachineSSAContext>::usesValueFromCycle(
     const MachineInstr &I, const MachineCycle &DefCycle) const {
   assert(!isAlwaysUniform(I));
   for (auto &Op : I.operands()) {
     if (!Op.isReg() || !Op.readsReg())
       continue;
     auto Reg = Op.getReg();
-    assert(Reg.isVirtual());
+
+    // FIXME: Physical registers need to be properly checked instead of always
+    // returning true
+    if (Reg.isPhysical())
+      return true;
+
     auto *Def = F.getRegInfo().getVRegDef(Reg);
     if (DefCycle.contains(Def->getParent()))
       return true;
   }
   return false;
 }
 
 // This ensures explicit instantiation of

llvm/test/Analysis/DivergenceAnalysis/AMDGPU/MIR/uses-value-from-cycle.mir

+# RUN: llc -mtriple=amdgcn-- -mcpu=gfx1030 -run-pass=print-machine-uniformity -o - %s 2>&1 | FileCheck %s
+---
+name:            f1
+tracksRegLiveness: true
+body:             |
+  ; CHECK-LABEL: MachineUniformityInfo for function: f1
+  bb.1:
+    %3:_(s32) = G_CONSTANT i32 0
+    %25:_(s32) = G_IMPLICIT_DEF
+  
+  bb.2:
+    %0:_(s32) = G_PHI %22(s32), %bb.5, %3(s32), %bb.1
+    %1:_(s32) = G_PHI %3(s32), %bb.1, %20(s32), %bb.5
+    %2:_(s32) = G_PHI %3(s32), %bb.1, %19(s32), %bb.5
+    %36:_(s32) = G_CONSTANT i32 0
+    %4:_(s1) = G_ICMP intpred(slt), %1(s32), %36
+  
+  bb.3:
+    successors: %bb.4(0x04000000), %bb.3(0x7c000000)
+  
+    %5:_(s32) = G_PHI %38(s32), %bb.3, %1(s32), %bb.2
+    %38:_(s32) = G_CONSTANT i32 0
+    G_BRCOND %4(s1), %bb.3
+    G_BR %bb.4
+  
+  bb.4:
+    successors: %bb.7, %bb.5
+  
+    %6:_(s32) = G_PHI %5(s32), %bb.3
+    %33:_(s1) = G_CONSTANT i1 true
+    %7:_(s64) = G_SEXT %2(s32)
+    %39:_(s32) = G_CONSTANT i32 2
+    %10:_(s64) = G_SHL %7, %39(s32)
+    %11:_(p0) = G_INTTOPTR %10(s64)
+    %13:_(s32) = G_LOAD %11(p0) :: (load (s32))
+    %37:_(s32) = G_CONSTANT i32 0
+    %14:sreg_32_xm0_xexec(s1) = G_ICMP intpred(slt), %13(s32), %37
+    %16:sreg_32_xm0_xexec(s32) = SI_IF %14(s1), %bb.5, implicit-def $exec, implicit-def $scc, implicit $exec
+    G_BR %bb.7
+  
+  bb.5:
+    successors: %bb.6(0x04000000), %bb.2(0x7c000000)
+    ; CHECK: DIVERGENT: %{{[0-9]}}: %{{[0-9]}}:_(s32) = G_PHI
+    ; CHECK: DIVERGENT: %{{[0-9]}}: %{{[0-9]}}:_(s32) = G_PHI
+    ; CHECK: DIVERGENT: %{{[0-9]}}: %{{[0-9]}}:_(s32) = G_PHI
+    ; CHECK-NOT: DIVERGENT: %{{[0-9]}}: %{{[0-9]}}:sreg_32_xm0_xexec(s32) = G_INTRINSIC intrinsic(@llvm.amdgcn.if.break)
+    %19:_(s32) = G_PHI %18(s32), %bb.7, %25(s32), %bb.4
+    %20:_(s32) = G_PHI %6(s32), %bb.7, %25(s32), %bb.4
+    %21:_(s1) = G_PHI %34(s1), %bb.7, %33(s1), %bb.4
+    G_INTRINSIC_W_SIDE_EFFECTS intrinsic(@llvm.amdgcn.end.cf), %16(s32)
+    %22:sreg_32_xm0_xexec(s32) = G_INTRINSIC intrinsic(@llvm.amdgcn.if.break), %21(s1), %0(s32)
+    SI_LOOP %22(s32), %bb.2, implicit-def $exec, implicit-def $scc, implicit $exec
+    G_BR %bb.6
+  
+  bb.6:
+    %24:_(s32) = G_PHI %22(s32), %bb.5
+    G_INTRINSIC_W_SIDE_EFFECTS intrinsic(@llvm.amdgcn.end.cf), %24(s32)
+    SI_RETURN
+  
+  bb.7:
+    %34:_(s1) = G_CONSTANT i1 false
+    %35:_(s32) = G_CONSTANT i32 1
+    %18:_(s32) = G_OR %2, %35
+    G_BR %bb.5
+
+...