[PATCH] D156301: [AMDGPU] Support FAdd/FSub global atomics in AMDGPUAtomicOptimizer.

Fri Aug 11 06:00:29 PDT 2023

foad added inline comments.

================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUAtomicOptimizer.cpp:760
   }
-  Mbcnt = B.CreateIntCast(Mbcnt, Ty, false);
+  Mbcnt = !isAtomicFloatingPointTy ? B.CreateIntCast(Mbcnt, Ty, false) : Mbcnt;

----------------
pravinjagtap wrote:
> foad wrote:
> > Might be clearer as:
> > `Mbcnt = isAtomicFloatingPointTy ? B.CreateUIToFP(Mbcnt, Ty) : B.CreateIntCast(Mbcnt, Ty, false);`
> > (instead of doing the fp cast on line 996) since in both cases we want to convert Mbcnt to type Ty.
> If we convert `Mbcnt` to `float` here, Integer comparison will fail at line no 869 
Then I suggest moving the casts (both int and fp cases) down to line 976.

Currently, for a 64-bit integer atomic, we will case mbcnt to i64 here, so the comparison on line 869 will be an i64 comparison. That is silly. There is no need for the comparison to be wider than i32.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156301