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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert accepted this revision.
jdoerfert added a comment.

LGTM, see my comment below.



================
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.
 
----------------
nlopes wrote:
> 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.
I tried to find a case where it was not covered but failed, thus I agree with your assessment.

I would (have) prefer(red) these cleanup changes to be in a separate commit because they are not directly related to `freeze`.


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

https://reviews.llvm.org/D29121