- Describe the new freeze instruction
- Make it explicit that branch on undef/poison is UB
|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.
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 :-)
|5420 ↗||(On Diff #85871)|
'``cond``' ```? And maybe quote both undef and poison.
|5472 ↗||(On Diff #85871)|
Inconsistent quoting here too.
|5539 ↗||(On Diff #85871)|
Looks good! Some thoughts.
Should this section have something similar to the poison section, referencing freeze?
Should this mention freeze?
I can see that the scalar restriction was lifter (good)
Either in Overview or in Semantics the undef and poison should be links to those langref sections.
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.
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
Can/should this be reworded then to avoid that misreading i just did?
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.
> I'm not sure what the statement means. It doesn't seem to add extra useful information.
The statement as written is incorrect. That's the reason for removing a few lines from the example below as well.
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.
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 .)
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.
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.")
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?
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.
@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.