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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
