[PATCH] D118632: [Clang][OpenMP] Add the codegen support for `atomic compare`

[Shilei Tian via Phabricator via cfe-commits]

tianshilei1992 added a comment.

In D118632#3336145 <https://reviews.llvm.org/D118632#3336145>, @ABataev wrote:

> In D118632#3336133 <https://reviews.llvm.org/D118632#3336133>, @tianshilei1992 wrote:
>
>> In D118632#3336094 <https://reviews.llvm.org/D118632#3336094>, @ABataev wrote:
>>
>>> LG with a nit
>>
>> Actually I got two question in the inline comments. Can you please take a look?
>
> Could you point me?

Sure.



================
Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:6041
+  llvm::Value *EVal = CGF.EmitScalarExpr(E->IgnoreImpCasts());
+  llvm::Value *DVal = D ? CGF.EmitScalarExpr(D->IgnoreImpCasts()) : nullptr;
+
----------------
tianshilei1992 wrote:
> Using `D->IgnoreImpCasts()` can make sure to avoid the case that `char` is casted to `int` in binary operation. However, say, if user writes the following code:
> ```
> int x;
> #pragma omp atomic compare
>   x = x > 1.01 ? 1.01 : x;
> ```
> `1.01` here will be casted to `1` by clang, and a warning will be emitted. Because we ignore the implicit cast, in Sema, it is taken as floating point value. However, we already told user that it is casted to `1`, which is a little weird to emit an error then.
@ABataev Here.


================
Comment at: clang/test/OpenMP/atomic_compare_codegen.cpp:1990
+// CHECK-NEXT:    [[DD:%.*]] = alloca double, align 8
+// CHECK-NEXT:    [[TMP0:%.*]] = load i8, i8* [[CE]], align 1
+// CHECK-NEXT:    [[TMP1:%.*]] = atomicrmw umin i8* [[CX]], i8 [[TMP0]] monotonic, align 1
----------------
tianshilei1992 wrote:
> tianshilei1992 wrote:
> > I think the `store` here is redundant. Is it because I'm using `CGF.EmitScalarExpr`?
> Oh, shoot. `load` here, instead of `store`.
And here. @ABataev 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118632