[PATCH] D144162: [AMDGPU] Replace LegacyDA with Uniformity Analysis in AnnotateUniformValues

[Sameer Sahasrabuddhe via Phabricator via llvm-commits]

sameerds added a comment.

In D144162#4134296 <https://reviews.llvm.org/D144162#4134296>, @ruiling wrote:

> I think for this specific case, we should report %8 as uniform, and the branch should also be uniform. But there seems something wrong in the uniform analysis, if you try `opt -passes='print<uniformity>'` with the test here. We will see both the condition %8 and the conditional branch `br i1 %8,...` are reported as divergent. but the isUniform() query return true for the branch instruction. I think there should be something wrong in uniform analysis. If the condition is divergent, I think the branch should also be divergent. Another issue I want to point out that the uniform analysis has a subtle difference with divergence analysis. There is some comment in https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/AMDGPU/AMDGPURewriteUndefForPHI.cpp#L13 to explain the issue. I think we need to fill the gap to switch to uniform analysis, otherwise we will regress code generation. The last time we discussed this, I think we want some target specific option in uniform analysis to match this behavior with divergence analysis.

I agree with that assessment, and pretty much found the same thing myself by comparing with the output of "print<divergence>". The uniformity analysis assumes a phi is divergent if it has an undef argument, but the divergence analysis does not make that assumption. The difference is in the call to PHINode::hasConstantValue() for divergence analysis and PHINode::hasConstantOrUndefValue() for uniformity analysis.

I think we should just fix the uniformity analysis to match the existing behaviour and revisit the issue later. I can have the patch ready by Monday EOD IST.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144162/new/

https://reviews.llvm.org/D144162