[PATCH] D102449: [WIP][Clang][OpenMP] Add the support for compare clause in atomic directive

[Carlo Bertolli via Phabricator via llvm-commits]

carlo.bertolli added a comment.

In D102449#2977711 <https://reviews.llvm.org/D102449#2977711>, @tianshilei1992 wrote:

> Another thing is how we deal with a corner case. Say the OpenMP code is written in the following way:
>
>   #pragma omp atomic compare
>     x = e < x ? e : x;
>
> That's how OpenMP spec defines the atomic operation. `x` is always in "else statement" of a conditional statement.
>
> Now we need to lower it to LLVM IR, which is `atomicrmw` operation. Based on the LLVM IR reference, it only supports the atomic operations that `x` is in the "then statement". For example: `x = x > e ? x : e`. See the `x` here is before `:`. In order to lower the OpenMP statement, we have to do a transformation. In order to swap `e` and `x`, we need to transform it to `x = e >= x : x : e`, a.k.a. `x = x <= e : x : e`. However, we don't have an atomic operation for `<=`. We only have `<`. So if `x != e`, the result is good.
>
> The incorrectness happens if `x == e`. Recall at the OpenMP statement, when `x == e`, the result should be `x = x`. But if we look at our lowered LLVM IR, `x = x < e : x : e`, when `x == e`, it becomes `x = e`, which doesn't conform with OpenMP spec.
>
> What should we do here?

There seems to be an imbalance in the OpenMP specs, where I would have written something like the followin (page 268 or 5.1 spec's):

> x = expr ordop x ? expr : x;
> x = x ordop expr ? **x : expr**;

I tried thinking about a compact solution to your problem, including trying to take advantage of the fact that when x == e we don't need to actually do the assignment (page 272 line 17 of 5.1 spec's and see below), but I was unsuccessful.
Did you consider special casing this transforming it into the equivalent:
#pragma omp critical
{ if (x > e) x = e; }
?

Remember:

> For forms where statement is cond-expr-stmt, if the result of the condition implies that the value of x
> does not change then the update may not occur.

The right thing to do would be to fix the spec's, but that can't happen until 5.2.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102449