From f6e22f2f63b4f44adb904d017313f5a286a3a89f Mon Sep 17 00:00:00 2001 From: Sameer Sahasrabuddhe Date: Mon, 20 Feb 2023 14:25:37 +0530 Subject: [PATCH] [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 --- llvm/include/llvm/ADT/GenericUniformityImpl.h | 9 ++++++++- llvm/include/llvm/CodeGen/MachineSSAContext.h | 2 +- llvm/include/llvm/IR/SSAContext.h | 2 +- llvm/lib/CodeGen/MachineSSAContext.cpp | 2 +- llvm/lib/IR/SSAContext.cpp | 4 ++-- .../Analysis/DivergenceAnalysis/AMDGPU/phi-undef.ll | 17 ++++++++--------- 6 files changed, 21 insertions(+), 15 deletions(-) diff --git a/llvm/include/llvm/ADT/GenericUniformityImpl.h b/llvm/include/llvm/ADT/GenericUniformityImpl.h index 62c9e9e..fd86c53 100644 --- a/llvm/include/llvm/ADT/GenericUniformityImpl.h +++ b/llvm/include/llvm/ADT/GenericUniformityImpl.h @@ -963,7 +963,14 @@ void GenericUniformityAnalysisImpl::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 31a192c..8bf963d 100644 --- a/llvm/include/llvm/CodeGen/MachineSSAContext.h +++ b/llvm/include/llvm/CodeGen/MachineSSAContext.h @@ -58,7 +58,7 @@ public: static void appendBlockTerms(SmallVectorImpl &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 7551adf..346c1fc 100644 --- a/llvm/include/llvm/IR/SSAContext.h +++ b/llvm/include/llvm/IR/SSAContext.h @@ -63,7 +63,7 @@ public: 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 6de8f8d..7e53ce4 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 e758a3f..bb31c96 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(&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 c7dade2..780d03b 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' -disable-output %s 2>&1 | FileCheck %s --check-prefixes=CHECK,LOOPDA -; RUN: opt -mtriple amdgcn-- -passes='print' -disable-output %s 2>&1 | FileCheck %s --check-prefixes=CHECK,CYCLEDA +; RUN: opt -mtriple amdgcn-- -passes='print' -disable-output %s 2>&1 | FileCheck %s +; RUN: opt -mtriple amdgcn-- -passes='print' -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! -- 2.7.4