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

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

In D117921#3269497 <https://reviews.llvm.org/D117921#3269497>, @durin42 wrote:

> In D117921#3262394 <https://reviews.llvm.org/D117921#3262394>, @nikic wrote:
>
>> Might it make more sense to implement this as an argument attribute? Would be a bit simpler I think, and I don't really see a disadvantage.
>
> I don't have any idea. @reames any opinion? I confess I'm not sure what this means for me.

I don't see anyway with argument align attributes to express the alignment of the return value of the call.  Specifically, the fact this is trying to express is "the returned pointer has at least the alignment of the integer param X".

>> If you do want this to be a function int attribute, then the parameter reference should be zero-based, not one-based, for consistency with allocsize.
>
> I can't do it zero-based, because there's no way to store a zero in an int attribute. I can go to the work of adding getters and setters everywhere and then pretend it's 0-based for the public API, but internally you'll fail assertions if you try to carry 0 as the payload of an int attr. If you want the getters and setters let me know, I guess? This will matter for one other attribute we need too (`mustfree(N)` - to indicate a pointer will be freed).

Er, this seems strongly suspect to me.  I have not looked carefully, but I'm pretty sure we do use zero based indexing in allocsize.  You should be able to parallel that structure.  (You don't need the argument packing bit, but at the same time it does no harm to reuse if it simplifies the code.)

You need tests for this.  Both IR, and bitcode parsing.

I would suggest focusing this change on the mechanics of round tripping the new attribute and defining it.  We can defer the use to a separate patch.



================
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>])``
----------------
This definition needs expanded.

Questions:
* Units?
* one argument, two arguments?
* assumptions about new memory?

See the alllocsize for example text.  


================
Comment at: llvm/lib/Analysis/MemoryBuiltins.cpp:227
   bool IsNoBuiltinCall;
+  // TODO(augie): I think this is where I sniff for the attribute on the call
   if (const Function *Callee = getCalledFunction(V, IsNoBuiltinCall))
----------------
Nope, would be same place we handle allocsize today.


================
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)) {
----------------
To parallel the allocsize path, we need builtin knowledge to overrule attributed knowledge.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117921