[PATCH] D82316: [LangRef] Add `frozen` attribute to documentation

[Juneyoung Lee via Phabricator via llvm-commits]

aqjune added inline comments.


================
Comment at: llvm/docs/LangRef.rst:1259
+    requirement include structures with padding bits, unions with active
+    members smaller than their size, and scalars which were never initialized.
+
----------------
eugenis wrote:
> jdoerfert wrote:
> > eugenis wrote:
> > > aqjune wrote:
> > > > nikic wrote:
> > > > > This description is a bit too frontend-centric. I'd suggest to at least add
> > > > > 
> > > > > > If the parameter is poison or contains undefined bits, the behavior is undefined.
> > > > > 
> > > > > to describe the actual IR semantic this has.
> > > > > 
> > > > > Also, can this be applied to return values? If so, it should say so.
> > > > For aggregate types, I think it is okay to relax this constraint to allowing aggregates with undef padding bits. It will be great if an aggregate value that is constructed with a chain of `v = insertvalue (insertvalue undef, const1, idx), const2, idx2` can have this attribute attached regardless of whether the aggregate type has paddings or not. Inspecting padding bits which are not accessible without using memory operation may be overly restrictive.
> > > > For my previous concern (giving a guarantee that it has no undef bit in memory representation), this should be needed in certain future cases, but not now maybe.
> > > Yes, that's a good point. From MSan point of view, if {i8, i32} is passed as a call argument, we don't care about the padding in the middle - the type definition has all we need to understand which bits must be checked for definedness, and we'd like the attribute to reflect that - i.e. the property of the IR value itself and not its storage representation.
> > > 
> > > (if it's coerced to i64, then we care).
> > > 
> > > 
> > Does this mean we still need more than `nopoison`?
> Sorry, I don't understand - are saying the attribute can be specified in terms of poison values and not undef? I don't think it would work, we need it to cover something that's loaded from an uninitialized memory location and passed to a function call as is, and those are undef, not poison.
Yes, we need an attribute for undef bits; a constraint for absence of poison may not be enough.
In the long term, I'd prefer merging the notion of undef value poison value into one (by leaving poison only), then nopoison should be enough. But seems like it is a pretty tough way to go.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82316