[llvm][Uniformity] A phi with an undef argument is not always divergent

The uniformity analysis treated an undef argument to phi to be distinct from any other argument, equivalent to calling PHINode::hasConstantValue() instead of PHINode::hasConstantOrUndefValue(). Such a phi was reported as divergent. This is different from the older divergence analysis which treats such a phi as uniform. Fixed uniformity analysis to match the older behaviour. The original behaviour was added to DivergenceAnalysis in D19013. But it is not clear if relying on the undef value is safe. The defined values are not constant per se; they just happen to be uniform and the non-constant uniform value may not dominate the PHI. Reviewed By: ruiling Differential Revision: https://reviews.llvm.org/D144254
Pierre-vh · Feb 20, 2023 · f6e22f2 · f6e22f2
1 parent ac84bc3
commit f6e22f2
Show file tree

Hide file tree

Showing 6 changed files with 21 additions and 15 deletions.
diff --git a/llvm/include/llvm/ADT/GenericUniformityImpl.h b/llvm/include/llvm/ADT/GenericUniformityImpl.h
@@ -963,7 +963,14 @@ void GenericUniformityAnalysisImpl<ContextT>::taintAndPushPhiNodes(
   LLVM_DEBUG(dbgs() << "taintAndPushPhiNodes in " << Context.print(&JoinBlock)
                     << "\n");
   for (const auto &Phi : JoinBlock.phis()) {
-    if (ContextT::isConstantValuePhi(Phi))
+    // FIXME: The non-undef value is not constant per se; it just happens to be
+    // uniform and may not dominate this PHI. So assuming that the same value
+    // reaches along all incoming edges may itself be undefined behaviour. This
+    // particular interpretation of the undef value was added to
+    // DivergenceAnalysis in the following review:
+    //
+    // https://reviews.llvm.org/D19013
+    if (ContextT::isConstantOrUndefValuePhi(Phi))
       continue;
     if (markDivergent(Phi))
       Worklist.push_back(&Phi);

diff --git a/llvm/include/llvm/CodeGen/MachineSSAContext.h b/llvm/include/llvm/CodeGen/MachineSSAContext.h
@@ -58,7 +58,7 @@ template <> class GenericSSAContext<MachineFunction> {
   static void appendBlockTerms(SmallVectorImpl<const MachineInstr *> &terms,
                                const MachineBasicBlock &block);
   MachineBasicBlock *getDefBlock(Register) const;
-  static bool isConstantValuePhi(const MachineInstr &Phi);
+  static bool isConstantOrUndefValuePhi(const MachineInstr &Phi);
 
   Printable print(const MachineBasicBlock *Block) const;
   Printable print(const MachineInstr *Inst) const;

diff --git a/llvm/include/llvm/IR/SSAContext.h b/llvm/include/llvm/IR/SSAContext.h
@@ -63,7 +63,7 @@ template <> class GenericSSAContext<Function> {
                                const BasicBlock &block);
 
   static bool comesBefore(const Instruction *lhs, const Instruction *rhs);
-  static bool isConstantValuePhi(const Instruction &Instr);
+  static bool isConstantOrUndefValuePhi(const Instruction &Instr);
   const BasicBlock *getDefBlock(const Value *value) const;
 
   Printable print(const BasicBlock *Block) const;

diff --git a/llvm/lib/CodeGen/MachineSSAContext.cpp b/llvm/lib/CodeGen/MachineSSAContext.cpp
@@ -56,7 +56,7 @@ MachineBasicBlock *MachineSSAContext::getDefBlock(Register value) const {
   return RegInfo->getVRegDef(value)->getParent();
 }
 
-bool MachineSSAContext::isConstantValuePhi(const MachineInstr &Phi) {
+bool MachineSSAContext::isConstantOrUndefValuePhi(const MachineInstr &Phi) {
   return Phi.isConstantValuePHI();
 }
 

diff --git a/llvm/lib/IR/SSAContext.cpp b/llvm/lib/IR/SSAContext.cpp
@@ -75,9 +75,9 @@ bool SSAContext::comesBefore(const Instruction *lhs, const Instruction *rhs) {
   return lhs->comesBefore(rhs);
 }
 
-bool SSAContext::isConstantValuePhi(const Instruction &Instr) {
+bool SSAContext::isConstantOrUndefValuePhi(const Instruction &Instr) {
   if (auto *Phi = dyn_cast<PHINode>(&Instr))
-    return Phi->hasConstantValue();
+    return Phi->hasConstantOrUndefValue();
   return false;
 }
 

diff --git a/llvm/test/Analysis/DivergenceAnalysis/AMDGPU/phi-undef.ll b/llvm/test/Analysis/DivergenceAnalysis/AMDGPU/phi-undef.ll
@@ -1,15 +1,14 @@
-; RUN: opt -mtriple amdgcn-- -passes='print<divergence>' -disable-output %s 2>&1 | FileCheck %s --check-prefixes=CHECK,LOOPDA
-; RUN: opt -mtriple amdgcn-- -passes='print<uniformity>' -disable-output %s 2>&1 | FileCheck %s --check-prefixes=CHECK,CYCLEDA
+; RUN: opt -mtriple amdgcn-- -passes='print<divergence>' -disable-output %s 2>&1 | FileCheck %s
+; RUN: opt -mtriple amdgcn-- -passes='print<uniformity>' -disable-output %s 2>&1 | FileCheck %s
 
 ; CHECK-LABEL: 'test1':
 ; CHECK: DIVERGENT: i32 %bound
-; CYCLEDA: DIVERGENT: %counter =
-; LOOPDA: {{^  *}} %counter =
-; CHECK-NEXT: DIVERGENT: %break = icmp sge i32 %counter, %bound
-; CYCLEDA: DIVERGENT: %counter.next =
-; CYCLEDA: DIVERGENT: %counter.footer =
-; LOOPDA: {{^  *}}%counter.next =
-; LOOPDA: {{^  *}}%counter.footer =
+; CHECK: {{^  *}} %counter =
+; CHECK: DIVERGENT: %break = icmp sge i32 %counter, %bound
+; CHECK: DIVERGENT: br i1 %break, label %footer, label %body
+; CHECK: {{^  *}}%counter.next =
+; CHECK: {{^  *}}%counter.footer =
+; CHECK: DIVERGENT: br i1 %break, label %end, label %header
 
 ; Note: %counter is not divergent!