[PATCH] D153953: Revert "[AMDGPU] Mark mbcnt as convergent"

Wed Jun 28 23:24:57 PDT 2023

pravinjagtap added a comment.

In D153953#4458134 <https://reviews.llvm.org/D153953#4458134>, @sameerds wrote:

> @pravinjagtap @arsenm ... reverting the mbcnt intrinsic affects tests that were added for atomic optimizations. In particular, the mbcnt is now being moved across/into/out of control flow because it is no longer convergent. I eyeballed one example and it seemed okay to me, but a more thorough check will be useful.

Hello @sameerds,

The existing logic (before D147408 <https://reviews.llvm.org/D147408>) in atomic optimizer uses `mbcnt(exec)` to setup the control flow to allow only one lane to update the reduced value [Please refer: https://github.com/llvm/llvm-project/blob/80155cbf0be1744953edf68b9729c24bd0de79c4/llvm/lib/Target/AMDGPU/AMDGPUAtomicOptimizer.cpp#L671]. The instructions related to that seems to be moved to other basic block (ComputeEnd). In my opinion it should not affect the logic. @ruiling and @foad can you please confirm this ?

While working on D147408 <https://reviews.llvm.org/D147408>, I introduced cttz(exec) [Please refer: https://github.com/llvm/llvm-project/blob/80155cbf0be1744953edf68b9729c24bd0de79c4/llvm/lib/Target/AMDGPU/AMDGPUAtomicOptimizer.cpp#L538C1-L539C72] which is not being affected here. The `EXEC` is being moved to sgpr before the `ComputeLoop` (which we want) and then used it inside the `ComputeLoop` and modified inside it to set up the logic for `ComputeLoop`. This is not being affected because of this change.

I could be completely wrong here. Lets see what others have to say.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153953