Page MenuHomePhabricator

[Docs] Add LangRef documention for freeze instruction
ClosedPublic

Authored by nlopes on Jan 25 2017, 3:16 AM.

Details

Summary
  1. Describe the new freeze instruction
  2. Make it explicit that branch on undef/poison is UB

Diff Detail

Event Timeline

nlopes created this revision.Jan 25 2017, 3:16 AM
nlopes added a subscriber: llvm-commits.
filcab added a subscriber: filcab.Jan 25 2017, 3:57 AM
filcab added inline comments.
LangRef.rst
8589 ↗(On Diff #85724)
All uses of a '``freeze``' instruction are ...

would probably be less ambiguous. (uses of the same freeze instruction vs two freeze instructions on the same value)

efriedma added inline comments.Jan 25 2017, 11:51 AM
LangRef.rst
5408 ↗(On Diff #85724)

Does "switch" need a similar amendment?

majnemer added inline comments.Jan 25 2017, 12:05 PM
LangRef.rst
5408 ↗(On Diff #85724)

I think indirectbr would need it too.

efriedma added inline comments.Jan 25 2017, 12:23 PM
LangRef.rst
5408 ↗(On Diff #85724)

indirectbr already says "All possible destination blocks must be listed in the label list, otherwise this instruction has undefined behavior", which implies that "indirectbr undef" is UB. Could be a bit more explicit, I guess.

nlopes updated this revision to Diff 85871.Jan 26 2017, 1:20 AM
  • rebase
  • add UB semantics to indirectbr and switch
nlopes marked 4 inline comments as done.Jan 26 2017, 1:21 AM

Please check the inconsistent quoting of keywords (which we already have in LangRef, but no need adding to it). Otherwise, no more nitpicking on my side :-)
Thanks.

LangRef.rst
5420 ↗(On Diff #85871)
'``cond``'
```?
And maybe quote both undef and poison.
5472 ↗(On Diff #85871)

Inconsistent quoting here too.

5539 ↗(On Diff #85871)

And here.

spatel added a subscriber: spatel.May 22 2017, 2:01 PM
delcypher added inline comments.
LangRef.rst
8598 ↗(On Diff #85871)

Should probably say scalar integer type. It would also be a good idea to explain why this restriction is in place.

lebedev.ri added a subscriber: lebedev.ri.

I'd think it would make sense to start with this patch in the series.
Needs rebasing, comments are not addressed.

LangRef.rst
8598 ↗(On Diff #85871)

Not done.

fhahn added a subscriber: fhahn.Aug 31 2019, 2:13 PM

reverse ping.

nlopes updated this revision to Diff 220296.Sep 16 2019, 2:23 AM

Reflect comments & clarify immediate UB when branching on poison.

Herald added a project: Restricted Project. · View Herald TranscriptSep 16 2019, 2:23 AM

Looks good! Some thoughts.

llvm/docs/LangRef.rst
3142–3143

Should this section have something similar to the poison section, referencing freeze?

3338–3342

Should this mention freeze?

10239

I can see that the scalar restriction was lifter (good)
I think this misses 2 examples:

  1. Some trivial vector example
  2. An example for All uses of a 'freeze' instruction are guaranteed to always observe the same value.
nlopes updated this revision to Diff 220306.Sep 16 2019, 4:18 AM

Updated with an example with vectors and add more references to freeze section.

lebedev.ri added inline comments.Sep 16 2019, 4:28 AM
llvm/docs/LangRef.rst
10217–10232

Either in Overview or in Semantics the undef and poison should be links to those langref sections.

10239

To clarify why i think there should be an example for All uses of a 'freeze' instruction are guaranteed to always observe the same value. - elsewhere it is specified that undef can 'evaluate' to a different value each time, and here we are contradicting that.

10241–10244

Could be as simple as:

diff
+%x2 = freeze i32 %w
+%z2 = add i32 %x, %x2 ; even number because all uses of %x and %x2 observe same value

Two comments on the diff, the freeze part looks good to me.

llvm/docs/LangRef.rst
3246

I feel "even if 'undef'" reads as if there is a word missing.

3350

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?

nlopes marked 2 inline comments as done.Sep 16 2019, 5:25 AM
nlopes added inline comments.
llvm/docs/LangRef.rst
3350

I'm not sure what the statement means. It doesn't seem to add extra useful information.

10241–10244

That is not correct. Only all uses of a given freeze instruction are guaranteed to have the same value.
Different freeze %x may yield different values.

lebedev.ri added inline comments.Sep 16 2019, 5:32 AM
llvm/docs/LangRef.rst
10231–10232

Can/should this be reworded then to avoid that misreading i just did?
I'd expect to see something closer to

All uses of a value returned from the same '``freeze``' instruction are
guaranteed to always observe the same value, but different '``freeze``'
instructions may yield different values.
10241–10244

Oh

nlopes updated this revision to Diff 220328.Sep 16 2019, 6:44 AM

Made it explicit re one vs multiple calls to freeze.

lebedev.ri accepted this revision.Sep 16 2019, 6:52 AM

Ok, this LG to me.
Would be good if someone else could also review.

This revision is now accepted and ready to land.Sep 16 2019, 6:52 AM
jdoerfert added inline comments.Sep 16 2019, 2:22 PM
llvm/docs/LangRef.rst
3350

> 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?
nlopes marked an inline comment as done.Sep 17 2019, 6:35 AM
nlopes added inline comments.
llvm/docs/LangRef.rst
3350

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.

jdoerfert accepted this revision.Sep 30 2019, 1:30 PM

LGTM, see my comment below.

llvm/docs/LangRef.rst
3350

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.

@nlopes anything holding this patch? do you intend to land them all at once?
@aqjune I suppose D29011 should be the next one up for review..

nlopes added a comment.Oct 5 2019, 6:19 AM

@nlopes anything holding this patch? do you intend to land them all at once?
@aqjune I suppose D29011 should be the next one up for review..

Yes, I would prefer if they would all land at once.

Should there be a constantexpr version of freeze?
Also, @nlopes, just to put a bold stop to this question - in the end,
we want freeze to be fully agnostic, it should not care *at all* what the type is, right?

we want freeze to be fully agnostic, it should not care *at all* what the type is, right?

Well, it has to be some value which actually has bits that can be frozen. So integers, floats, pointers, vectors, arrays, and structs. Maybe worth listing out explicitly.

For pointers, we should probably mention the aliasing rules explicitly. It should be similar to null: the result can't be dereferenced. (See http://llvm.org/docs/LangRef.html#pointer-aliasing-rules .)

nlopes updated this revision to Diff 223673.Oct 7 2019, 3:17 PM

Clarify semantics for pointers.

LGTM w/minor entirely optional comments.

Personally, I'd prefer to see the addition of freeze land, then the optimizer fixes, then the changing of the branch being full UB wording, but I won't insist on that.

llvm/docs/LangRef.rst
3353–3355

Suggested minor tweak to wording: integrate this as a bullet item into the previous list. (i.e. something like "* The condition operand to a branch, select. * The callee operand of a call or invoke.")

10235

I think this paragraph could be dropped. It's implied by the previous definition and oddly specific. Why does this particular implication deserve spelling out and not others?

So this is not committed till we reviewed all of them?

So this is not committed till we reviewed all of them?

I'm just waiting on SelDAG->MIR patch to be done (https://reviews.llvm.org/D29014).
Then we have docs, SelDAG & MIR support. That seems the minimal to me (so the compiler won't crash when freeze is used). I agree the optimizer patches can land later.

So this is not committed till we reviewed all of them?

I'm just waiting on SelDAG->MIR patch to be done (https://reviews.llvm.org/D29014).
Then we have docs, SelDAG & MIR support. That seems the minimal to me (so the compiler won't crash when freeze is used). I agree the optimizer patches can land later.

Can I suggest that we add a stub* ISEL implementation, land this, and then build on top of it?

  • By which I mean a knowingly incorrect lowering to a simple COPY. It wouldn't fix *all* of our poison/undef problems, but it would allow us to make progress on some of the most painful IR ones while waiting for that patch to land.

So this is not committed till we reviewed all of them?

I'm just waiting on SelDAG->MIR patch to be done (https://reviews.llvm.org/D29014).
Then we have docs, SelDAG & MIR support. That seems the minimal to me (so the compiler won't crash when freeze is used). I agree the optimizer patches can land later.

Can I suggest that we add a stub* ISEL implementation, land this, and then build on top of it?

  • By which I mean a knowingly incorrect lowering to a simple COPY. It wouldn't fix *all* of our poison/undef problems, but it would allow us to make progress on some of the most painful IR ones while waiting for that patch to land.

Makes sense. We are trying to get commit access to Juneyoung, but we were caught in the middle of the GitHub transition :) It's unclear what the procedure is now -- working on it.

Updated developer policy is at https://github.com/llvm/llvm-project/blob/master/llvm/docs/DeveloperPolicy.rst . (Updating the website is currently broken.)

This revision was automatically updated to reflect the committed changes.
RKSimon added a subscriber: RKSimon.Sat, Nov 9, 2:32 PM

@nlopes This is failing on the llvm-sphinx-docs buildbot, please can you take a look? http://lab.llvm.org:8011/builders/llvm-sphinx-docs/builds/37793/steps/docs-llvm-html/logs/stdio

Warning, treated as error:
/home/buildbot/llvm-build-dir/llvm-sphinx-docs/llvm/src/llvm/docs/LangRef.rst:10243: WARNING: Could not lex literal_block as "llvm". Highlighting skipped.

@nlopes This is failing on the llvm-sphinx-docs buildbot, please can you take a look? http://lab.llvm.org:8011/builders/llvm-sphinx-docs/builds/37793/steps/docs-llvm-html/logs/stdio

Warning, treated as error:
/home/buildbot/llvm-build-dir/llvm-sphinx-docs/llvm/src/llvm/docs/LangRef.rst:10243: WARNING: Could not lex literal_block as "llvm". Highlighting skipped.

I noticed, but when I run locally there were so many warnings that I couldn't get to this one.
I guess I'll just remove the code highlighting.
Thanks!