[PATCH] D147719: [OpenMP] Replace HeapToShared's initial value with `poison`

Fri Apr 7 12:41:29 PDT 2023

jdoerfert added a subscriber: nikic.
jdoerfert added a comment.

In D147719#4249322 <https://reviews.llvm.org/D147719#4249322>, @tra wrote:

> In D147719#4249298 <https://reviews.llvm.org/D147719#4249298>, @jhuber6 wrote:
>
>> This was in reponse to https://reviews.llvm.org/D147572 suggesting that we use `poison` in general over `undef`. I feel the semantics are roughly equivalent here, so I don't think this patch changes any existing behavior. All of these values get `addrspacecast`ed before they're used, but as far as I know those simply propagate poison values and don't imply any sort of volatility.
>
> I'm somewhat concerned about this property of poison <https://llvm.org/docs/LangRef.html#poison-values>: "It is correct to replace a poison value with an undef value or any value of the type."
>
> The way I read it, poison *may* make it possible to assume whatever value is convenient for optimization passes, which is not what we want for the values read from shared memory.

Your example is not impacted by the Undef -> Poison change. What you describe is either a valid transformation for both, or invalid for both.

The example from before cannot be optimized as is, but if you make `x` internal, we already propagate the value just fine (with undef as initializer):
https://godbolt.org/z/bo3zn56jM

That said, I can see that we might want to be consistent with other loads from uninitialized memory.
As of right now, this mostly results in undef, not poison. 
While only slightly different, it basically prevents people from comparing the undef values loaded from uninitialized memory while getting "sane" results.

@nlopes @nikic I assume loading uninitialized memory is and will remain undef?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147719