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

[Shilei Tian via Phabricator via llvm-commits]

tianshilei1992 marked an inline comment as done.
tianshilei1992 added inline comments.


================
Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:6041
+  llvm::Value *EVal = CGF.EmitScalarExpr(E->IgnoreImpCasts());
+  llvm::Value *DVal = D ? CGF.EmitScalarExpr(D->IgnoreImpCasts()) : nullptr;
+
----------------
ABataev wrote:
> tianshilei1992 wrote:
> > 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.
> Sorry, did not understand it was a question. Could you provide a bit more background, did not quite understand problem?
I have a better explain in another inline comment in Sema.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:11231
     C = Cond;
-    D = CO->getTrueExpr();
+    D = CO->getTrueExpr()->IgnoreImpCasts();
     if (checkIfTwoExprsAreSame(ContextRef, X, Cond->getLHS())) {
----------------
Here we do `D->IgnoreImpCasts()`, as shown in the code. The reason is, if we have two `char`s, say `char a, b`, and when `a > b`, clang also inserts an implicit cast from `char` to `int` for `a` and `b`. We want the actual type here otherwise in the IR builder, the type of `D` doesn't match `X`'s, causing issues.
However, that `IgnoreImpCasts` here can cause another issue. Let's say we have the following code:
```
int x;
#pragma omp atomic compare
  x = x > 1.01 ? 1.01 : x;
```
clang will also insert an implicit cast from floating point value to integer for the scalar value `1.01` (corresponding to `D` in the code), and also emits a warning saying the floating point value has been cast to integer or something like that. Because we have the `IgnoreImpCasts` now, it will fail the type check because `D` now is of floating point type, and we will emit an error to user.
For users, it might be confusing because on one hand we emit a warning saying the floating point value has been casted, and on the other hand in Sema we tell users it is expected an integer. Users might be thinking, you already cast it to integer, why do you tell me it's a floating point value again?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118632