[PATCH] D29121: [Docs] Add LangRef documention for freeze instruction

[Nuno Lopes via Phabricator via llvm-commits]

nlopes marked an inline comment as done.
nlopes added inline comments.


================
Comment at: llvm/docs/LangRef.rst:3318
-Additionally, undefined behavior occurs if a side effect *depends* on poison.
-This includes side effects that are control dependent on a poisoned branch.
 
----------------
jdoerfert wrote:
> nlopes wrote:
> > jdoerfert wrote:
> > > Why did we get rid of the side effect on poison is UB stuff here? In the absence of freeze this is still the case, isn't it?
> > I'm not sure what the statement means. It doesn't seem to add extra useful information.
>  > I'm not sure what the statement means. It doesn't seem to add extra useful information.
> 
> 1) Assuming it is not wrong, why remove it with this diff?
> 2) Is there some other location that specifies that a side effect depending on poison is UB?
The statement as written is incorrect. That's the reason for removing a few lines from the example below as well.
Branching on poison is UB straight away, while this paragraph and the example suggest that it would only be poison if there were side-effecting operations in the destination BB.  This semantics doesn't allow GVN to exist.
The paragraph before already mentions the interaction between instructions that trigger UB and poison.
Given these 2 statements (branch on poison, poison with UB-triggering instructions), I don't think there's anything else to cover. That's why I've removed it. Leaving stuff behind creates confusion, since it will otherwise create the idea that there are more cases that those 2 presented, but are left unspecified.
For example, I don't see any problem in printing a poison value. It will just print garbage, but it's not UB /per se/. It might be if the printing function branches on the value, for example.

It is important to cleanup the definition of poison so that people know when `freeze` is needed or not. The two concepts are very intermingled.


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

https://reviews.llvm.org/D29121