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

[James Y Knight via Phabricator via llvm-commits]

jyknight added inline comments.


================
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>])``
----------------
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.


================
Comment at: llvm/lib/Analysis/MemoryBuiltins.cpp:339
 
-  const Optional<AllocFnsTy> FnData = getAllocationData(V, AnyAlloc, TLI);
-  if (!FnData.hasValue() || FnData->AlignParam < 0) {
-    return nullptr;
+  const CallBase *CB = cast<CallBase>(V);
+  if (CB->hasFnAttr(Attribute::AllocAlign)) {
----------------
reames wrote:
> To parallel the allocsize path, we need builtin knowledge to overrule attributed knowledge.  
Why does allocsize work that way? I'd generally expect an attribute to take precedence if specified?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117921