[PATCH] D117921: Attributes: add a new allocalign attribute

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

In D117921#3294002 <https://reviews.llvm.org/D117921#3294002>, @reames wrote:

> This patch looks mechanically fine, we just need to get the LangRef wording finalized.
>
> I did have a thought when reading over this.  Is there anything specific here to an allocation function?  Could we generalize this into a generic "retalign" parameter attribute which generalizes align(N) on the return value and allows non-constant alignments to be described?
>
> I'm not sure that generalization is actually useful, it just seems like we have something which is almost non-allocation specific.  If we could think of examples where non-constant alignments are useful, then it seems a shame to leave it specialized...

I agree that the actual semantics here should be independent of whether the function is also, in some sense, considered an "allocator". I'd be fine still calling it `allocalign` in that case (similar to how `allocsize` is a general statement that the number of bytes is dereferenceable_or_null). `retalign` also sounds reasonable though.



================
Comment at: llvm/docs/LangRef.rst:1548
+    This attribute indicates the 1-indexed function parameter that specifies
+    the alignment of the newly allocated memory.
 ``allocsize(<EltSizeParam>[, <NumEltsParam>])``
----------------
jyknight wrote:
> nikic wrote:
> > jyknight wrote:
> > > reames wrote:
> > > > This definition needs expanded.
> > > > 
> > > > Questions:
> > > > * Units?
> > > > * one argument, two arguments?
> > > > * assumptions about new memory?
> > > > 
> > > > See the alllocsize for example text.  
> > > I don't think the allocsize description is a great example of clarity...
> > > 
> > > For allocsize, I'd want to know how it relates to -- or differs from -- placing `dereferenceable_or_null(value-of-EltSizeParam)` or `dereferenceable_or_null(value-of-EltSizeParam * value-of-NumEltsParam)` on the function return. Other than that the latter is only expressible when the values are constants, of course.
> > > 
> > > It is not at all clear from the wording, but it's treated completely differently internally right now. (Maybe it shouldn't actually be? Dunno! The current description doesn't seem to suggest any of the semantics we've actually applied to it!)
> > > 
> > > Anyways, for allocalign, I think we can probably do better.
> > > 
> > > E.g, maybe we can say "This attribute is equivalent to placing `align(value-of-allocalign-parameter)` on the function return, except that the value is not required to be a constant, and <something about non-power-of-2 values>." -- and actually have the code treat it that way, as well.
> > > 
> > > So, what _should_ the semantics be if the parameter is not a power-of-2?
> > > - UB.
> > > - Ignore (treat as if alignment was unknown -- e.g. 1)
> > > - Something else?
> > > 
> > > Looks like we're currently inconsistent in the handling at the moment for the hardcoded functions. AttributorAttributes.cpp seems to be assuming power-of-2 (and tries to create an invalid alloca instruction if it's not), while InstCombineCalls.cpp ignores the specified alignment if it's not a power-of-2. (Am I the only one surprised that "Attributor" can make very-much non-attribute IR changes like doing heap-to-stack!)
> > > 
> > > I think I'd vote for "ignore".
> > > 
> > > Units: Yea, clearly this is "alignment in bytes", but that should be specified.
> > I would probably go for UB here, in the interest of allowing handling of non-constant alignments by creating assumes. LangRef is not clear on this, but I expect that an align operand bundle assume with a non-power-of-two value would be UB.
> > 
> > No strong preference though.
> On second look, this must NOT be UB.
> 
> Per C2x aligned_alloc:
> "If the value of alignment is not a valid alignment supported by the implementation the function shall fail by returning a null pointer." (and elsewhere it says "Every valid alignment value shall be a nonnegative integral power of two.")
> 
> We cannot turn a guarantee of returning nullptr into UB. We must be prepared to accept any alignment at runtime, even if we ignore non-power-of-2 ones.
Good point. In that case I'm fine with saying that non-power-of-two alignment is ignored. Alternatively we could specify that `ptrtoint(ret) % param == 0`, which would be the non-power-of-two generalization (and trivially holds for the null pointer), but I don't think there is anything useful we could do with that, and it's probably best to stick with the well-defined notion of alignment (don't want to bring question like "what about non-integral pointers?" into this.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117921