[PATCH] D63044: [LangRef] Clarify poison semantics

[Nuno Lopes via Phabricator via llvm-commits]

nlopes added inline comments.


================
Comment at: llvm/docs/LangRef.rst:3270
+   any other pointer dereferencing instruction, if dereferencing ``null``
+   is undefined behavior based on address space and function attributes.
+-  The divisor operand of a ``udiv``, ``sdiv``, ``urem`` or ``srem``
----------------
jdoerfert wrote:
> Why do you make `null` special here? Generally, there are a lot of non-dereferenceable addresses and `null` is just a special one. I'd argue, loading poison, etc., is bad even if `null` is a valid address.
I agree. Dereferencing poison should always be UB, since that pointer is not based on a valid object.


================
Comment at: llvm/docs/LangRef.rst:3275
+Additionally, undefined behavior occurs if a side effect, including any
+:ref:`volatile <volatile>` operation, depends on poison.
 
----------------
nikic wrote:
> nlopes wrote:
> > This implies that doing a volatile store of a poison to memory is UB. Is this intended? I don't know of a use case that requires such behavior.
> One of the examples below explicitly lists volatile store of poison value as UB, which is why I mentioned it here:
> 
> ```
>       store volatile i32 %poison, i32* @g  ; External observation; undefined behavior.
> ```
> 
> The only code I'm aware of that does does reasoning in terms of side-effects with (control) dependence on poison is https://github.com/llvm-mirror/llvm/blob/1cbbb3f527a5aa13eda990e3dfa31f2ca4f64e07/lib/Analysis/ScalarEvolution.cpp#L6010-L6030, which does not care about the classification of any particular operation.
> 
> Personally I don't think it makes sense to classify a volatile store of poison as UB for two reasons: First, a volatile operation //can// have side-effects, but does not need to. If it does have side-effects then it is UB, but for the purposes of generic IR reasoning, we have to assume here that it might just be a store to normal memory that happens to be marked volatile. Second, making an operation `volatile` generally allows strictly less transformations to be performed. This would be a case where marking something `volatile` would allow more aggressive optimizations using poison-based reasoning, which does not seem right.
> 
> I'd be happy to drop the provision about volatile operations here, as well as the example below.
Thank you, sounds good. I don't see why external observability actually matters for poison.
I agree with removing this part.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63044