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

[Sameer Sahasrabuddhe via llvm-commits]

Author: Sameer Sahasrabuddhe
Date: 2023-02-20T14:26:43+05:30
New Revision: f6e22f2f63b4f44adb904d017313f5a286a3a89f

URL: https://github.com/llvm/llvm-project/commit/f6e22f2f63b4f44adb904d017313f5a286a3a89f
DIFF: https://github.com/llvm/llvm-project/commit/f6e22f2f63b4f44adb904d017313f5a286a3a89f.diff

LOG: [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

Added: 
    

Modified: 
    llvm/include/llvm/ADT/GenericUniformityImpl.h
    llvm/include/llvm/CodeGen/MachineSSAContext.h
    llvm/include/llvm/IR/SSAContext.h
    llvm/lib/CodeGen/MachineSSAContext.cpp
    llvm/lib/IR/SSAContext.cpp
    llvm/test/Analysis/DivergenceAnalysis/AMDGPU/phi-undef.ll

Removed: 
    


################################################################################
diff  --git a/llvm/include/llvm/ADT/GenericUniformityImpl.h b/llvm/include/llvm/ADT/GenericUniformityImpl.h
index 62c9e9ec74520..fd86c533f4e27 100644
--- 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
index 31a192cd8d290..8bf963da9748f 100644
--- 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
index 7551adff1e127..346c1fc67574c 100644
--- 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
index 6de8f8da92547..7e53ce4d68615 100644
--- 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
index e758a3fbeac9f..bb31c967c6540 100644
--- 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
index c7dade2afedc9..780d03b987f01 100644
--- 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!