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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

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

> In D147719#4251944 <https://reviews.llvm.org/D147719#4251944>, @jdoerfert wrote:
>
>> Your example is not impacted by the Undef -> Poison change. What you describe is either a valid transformation for both, or invalid for both.
>
> Are they not impacted because it "happens to work" now, or because both undef and poison are guaranteed to do the right thing here? That's what I'm unsure about, as poison sounds like it would give the compiler more freedom to optimize, while I'm afraid that it may already have too much freedom as it is.

There is only few real differences, e.g., if you have `icmp <op> undef, <val>` you get a result that is not undef but with poison you get poison.
For the transformation I was talking about, the one you described and my example showcased, it does not matter though.
We can assume `x` is 99 because it is otherwise either undef or poison and for the latter two we can just "pick" 99 too. So, we'll always load `99` is correct in both cases.

>> 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
>
> Exactly. That's where GPUs throw a wrench in LLVM's view of the world. While that variable may be internal to the module, as far as the symbol visibility is concerned, it is accessible from the other threads and may be modified by them.
> The example above produces wrong code as nothing ever will be stored in `x`, before we load from it.

You lost me. If nothing is stored in `x`, `99` is a perfectly fine value to "read" from `x`. The optimization is sound, IMHO.
Feel free to modify the example and cause it to break, we use this in production so finding bugs is good ;)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147719