[Uniformity] Avoid marking all PHIs as divergent in join blocks #157808

perlfu · 2025-09-10T08:02:50Z

Attempt to avoid marking PHIs divergent in join blocks. Only mark PHIs which contain values with sync dependence on the divergent terminator condition.

dstutt

Minor things...

llvm/include/llvm/ADT/GenericUniformityImpl.h

nhaehnle

Some inline comments, but I think they ultimately boil down to this:

What I suspect the change should really be doing is to open-code isConstantOrUndefValuePhi, but skip over predecessors that aren't on any of the diverged paths.

Does that make sense?

llvm/include/llvm/ADT/GenericUniformityImpl.h

perlfu · 2025-09-25T09:51:51Z

What I suspect the change should really be doing is to open-code isConstantOrUndefValuePhi, but skip over predecessors that aren't on any of the diverged paths.

I agree. I already had the feeling that half of this was open coding isConstantOrUndefValuePhi.
I have reworked the methodology inline with that.

There is now one test change where I PHI becomes uniform.
I think this case is correct, but have not spent too much time on it.

One oddity is that hasConstantOrUndefValue normally ignores self references in PHIs.
I found that apply that logic here produced uniformity results which I didn't agree with.
I have left comments in that indicate where this logic should be, but will remove them if this is merged.

jayfoad · 2025-09-25T10:00:48Z

@ssahasra it would be great if you could take a close look at this.

llvm/include/llvm/ADT/GenericSSAContext.h

llvm/lib/CodeGen/MachineSSAContext.cpp

jayfoad · 2025-09-25T10:14:54Z

llvm/test/Analysis/UniformityAnalysis/AMDGPU/hidden_loopdiverge.ll

+  %merge.y = phi i32 [ 42, %X ], [ %b, %B ]
  ret void
-; CHECK: DIVERGENT: %div.merge.y =
+; CHECK-NOT: DIVERGENT: %merge.y =


I think this change is wrong. Once we go entry->H->G, there is a divergent branch which can choose either G->X->Y or G->L->C->H->B->Y (!). Therefore the choice of whether we arrive at Y from X or from B is divergent.

I've fixed this by reinstating my previous code which respected temporal divergence.

llvm/lib/IR/SSAContext.cpp

- Address reviewer comments

llvm/include/llvm/ADT/GenericUniformityImpl.h

PankajDwivedi-25 · 2025-10-03T08:38:10Z

llvm/include/llvm/ADT/GenericUniformityImpl.h

+      // Track common value for all inputs.
+      if (!PhiCommon)
+        PhiCommon = Values[I];
+      else if (Values[I] != *PhiCommon)
+        HasSingleValue = false;
+
+      // Only consider predecessors on divergent path.
+      if (Blocks[I] != &DivTermBlock &&
+          !DivDesc.BlockLabels.lookup_or(Blocks[I], nullptr))
+        continue;
+
+      // Phi uniformity is maintained if all values on divergent path match.
+      if (!PathCommon)
+        PathCommon = Values[I];
+      else if (Values[I] != *PathCommon)


Not able to understand the intuition behind considering the values on a divergent path here? Isn't HasSingleValue = false sufficient here for marking it divergent?

if we really don't want to consider a non-divergent path, can we skip that early to make logic cleaner?

HasSingleValue here exists because this loop has absorbed the logic from isConstantOrUndefValuePhi.
We want to track both if the PHI has a single common value across all inputs, or failing that, if it has a single value across all predecessors on the same divergent path.
It should be uniform in both cases -- the existing code only handles the trivial case that the PHI only has a single value across all inputs.
The new test cases provide examples of this uniformity.

PankajDwivedi-25 · 2025-10-03T08:53:03Z

In the case when all the divergent path has the same value and `hasSingleValue = false', are we considering it as uniform?

ssahasra · 2025-10-06T05:48:39Z

llvm/include/llvm/ADT/GenericUniformityImpl.h

+
+    // Allow an empty Blocks/Values list to signify getPhiInputs is not
+    // implemented; in which case no uniformity is possible.
+    bool Uniform = !Values.empty();


Can't we just exit early at this point instead of going through the for-loop below over all incoming blocks?

Loop is already predicated on Uniform so it won't go through blocks in this case.

ssahasra · 2025-10-06T05:55:54Z

llvm/include/llvm/ADT/GenericUniformityImpl.h

+      if (!PathCommon)
+        PathCommon = Values[I];
+      else if (Values[I] != *PathCommon)
+        Uniform = false;


We can break here and just skip the remaining blocks. It might be possible to eliminate the Uniform variable if we moved the for-loop into a separate function or lambda with early returns.

We can only break here if HasSingleValue is also false -- the loop condition will handle this.

ssahasra · 2025-10-06T05:56:11Z

llvm/include/llvm/ADT/GenericUniformityImpl.h

+        Uniform = false;
+
+      // Phi is reached via divergent exit (i.e. respect temporal divergence).
+      if (DivDesc.CycleDivBlocks.contains(Blocks[I]))


Maybe check this first and avoid all other work?

I can move it up in the loop a bit.

ssahasra · 2025-10-06T06:01:20Z

llvm/lib/IR/SSAContext.cpp

+  for (unsigned I = 0, E = Phi->getNumIncomingValues(); I != E; ++I) {
+    const Value *Incoming = Phi->getIncomingValue(I);
+    const BasicBlock *Block = Phi->getIncomingBlock(I);
+    // FIXME: should this also consider Incoming == &Instr undef?


If all the dominating inputs are well-defined, then this self-reference is not undef, right? If at this point we find that indeed Incoming == &Instr, then I am not able to decide if treating it as undef is always correct. Ultimately, it the uniformity itself has to be false unless proven otherwise.

ssahasra · 2025-10-06T06:11:25Z

llvm/test/Analysis/UniformityAnalysis/AMDGPU/phi_no_divergence.ll

@@ -0,0 +1,114 @@
+; RUN: opt %s -mtriple amdgcn-- -passes='print<uniformity>' -disable-output 2>&1 | FileCheck %s
+


Would be useful to have one or two lines explaining what's the distinguishing pattern in each function compared to other functions in the file.

ssahasra · 2025-10-06T06:12:07Z

llvm/test/Analysis/UniformityAnalysis/AMDGPU/phi_no_divergence.ll

+; CHECK: DIVERGENT: %div.val =
+  ret void
+}
+


Do we need a test with temporal divergence?

I think the existing tests cover this.

perlfu · 2025-10-06T08:48:59Z

I have refactor the loop, and change variable naming to try and make its function more obvious.

ssahasra · 2025-10-06T09:31:18Z

llvm/include/llvm/ADT/GenericUniformityImpl.h

+
+      // Phi is reached via divergent exit (i.e. respect temporal divergence).
+      if (DivDesc.CycleDivBlocks.contains(Blocks[I])) {
+        UniformOnPath = false;


At this point, HasSingleValue might still be true, and then the PHINode will not get marked as divergent. But temporal divergence means that even the same input can have different values for different threads. Either this check should happen early to mark the PHINode divergent, or in fact this check is not needed at all, because eventually UA will mark all temporally divergent uses anyway when it calls propagateTemporalDivergence().

At this point, HasSingleValue might still be true, and then the PHINode will not get marked as divergent.
Yes, this is what the current implementation prior to this patch does.

The key difference between this test and propagateTemporalDivergence is whether it is related to the CFG edge or the value definition.
propagateTemporalDivergence only marks PHIs if they use a value defined within a loop.
This marks the PHI based on its reachability from a divergent exit.

The test hidden_doublebreak_diverge in hidden_loopdiverge.ll highlights this.
The PHI %div.merge.y = phi i32 [ 42, %X ], [ %b, %B ] only uses values defined outside the function, so the values themselves will not cause temporal divergence.

To borrow Jay's summary:

Once we go entry->H->G, there is a divergent branch which can choose either G->X->Y or G->L->C->H->B->Y. Therefore the choice of whether we arrive at Y from X or from B is divergent.

The divergent path via B only has a single value (%b), so this PHI would not be marked divergent (UniformOnPath = true) unless we consider the temporal divergence of the path as well.

llvm/include/llvm/ADT/GenericUniformityImpl.h

[Uniformity] Avoid marking all PHIs as divergent in join blocks

7c44e3b

Attempt to avoid marking PHIs divergent in join blocks. Only mark PHIs which contain values with sync dependence on the divergent terminator condition.

perlfu requested review from jayfoad, nhaehnle and ssahasra September 10, 2025 08:02

dstutt reviewed Sep 10, 2025

View reviewed changes

llvm/include/llvm/ADT/GenericUniformityImpl.h Outdated Show resolved Hide resolved

llvm/include/llvm/ADT/GenericUniformityImpl.h Show resolved Hide resolved

nhaehnle reviewed Sep 10, 2025

View reviewed changes

llvm/include/llvm/ADT/GenericUniformityImpl.h Outdated Show resolved Hide resolved

llvm/include/llvm/ADT/GenericUniformityImpl.h Outdated Show resolved Hide resolved

llvm/include/llvm/ADT/GenericUniformityImpl.h Outdated Show resolved Hide resolved

- Rework based on reviewer feedback

2804480

jayfoad reviewed Sep 25, 2025

View reviewed changes

llvm/include/llvm/ADT/GenericSSAContext.h Show resolved Hide resolved

jayfoad reviewed Sep 25, 2025

View reviewed changes

llvm/lib/CodeGen/MachineSSAContext.cpp Outdated Show resolved Hide resolved

jayfoad reviewed Sep 25, 2025

View reviewed changes

- Address reviewer comments

ca07077

jayfoad reviewed Sep 29, 2025

View reviewed changes

llvm/lib/IR/SSAContext.cpp Outdated Show resolved Hide resolved

- Consider Phi with common value and undefs uniform

f5fdd89

- Address reviewer comments

PankajDwivedi-25 reviewed Oct 3, 2025

View reviewed changes

ssahasra reviewed Oct 6, 2025

View reviewed changes

- Address reviewer comments

d2f469a

ssahasra reviewed Oct 6, 2025

View reviewed changes

- Incorporate reviewer feedback

ce186d5

		@@ -0,0 +1,114 @@
		; RUN: opt %s -mtriple amdgcn-- -passes='print<uniformity>' -disable-output 2>&1 \| FileCheck %s

[Uniformity] Avoid marking all PHIs as divergent in join blocks #157808

Are you sure you want to change the base?

[Uniformity] Avoid marking all PHIs as divergent in join blocks #157808

Uh oh!

Conversation

perlfu commented Sep 10, 2025

Uh oh!

dstutt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nhaehnle left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

perlfu commented Sep 25, 2025

Uh oh!

jayfoad commented Sep 25, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PankajDwivedi-25 commented Oct 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

perlfu commented Oct 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!