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

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/docs/LangRef.rst:1546
     parentheses.
+``allocalign``
+    This attribute marked function parameter is the alignment in bytes of the newly
----------------
This shouldn't be in the "function attributes" section.


================
Comment at: llvm/docs/LangRef.rst:1547
+``allocalign``
+    This attribute marked function parameter is the alignment in bytes of the newly
+    allocated block returned by this function. That is, it's similar to putting `align(N)`
----------------
"The function parameter marked with this attribute is [...]" maybe? The current phrasing reads odd to me.


================
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:
> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117921