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

[Alexey Bataev via Phabricator via cfe-commits]

ABataev 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;
+
----------------
tianshilei1992 wrote:
> 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.
If you don't need it, do not call it. Why do you need this rvalue?


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:11231
     C = Cond;
-    D = CO->getTrueExpr();
+    D = CO->getTrueExpr()->IgnoreImpCasts();
     if (checkIfTwoExprsAreSame(ContextRef, X, Cond->getLHS())) {
----------------
tianshilei1992 wrote:
> 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?
Yiu have the resulting type from the whole expression. You van do explicit cast to this resulting type to avoid problems, if I got it correctly 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118632