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

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

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

> It does use zero-based indexing, but we'll have to do custom getters/settings across the board like allocsize if we want to use zero-based indexing in both allocalign and mustfree. If that's the preference, I'm happy to do it, but it does seem like a fair amount of extra code for only marginal benefit.

If the existing infrastructure truly doesn't allow zero as a value for an attribute as an expressability level, then it's over specialized for existing attributes, and we should generalize.

With @nikic's clarification, I now understand what he meant, and agree that looks reasonable as well.



================
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)) {
----------------
durin42 wrote:
> jyknight wrote:
> > 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?
> I'm new to compilers at all and LLVM in particular, but I really would have expected an attribute to overrule builtin knowledge too, to the point that I didn't check how allocsize was set up. For now I'm making it consistent, but it does seem backwards.
> 
> Also, the "use the attribute" part of this has moved to a new change.
I have no idea why the code is structured the current way, but having the two *differ* seems like a recipe for confusion and disaster.  

If you want to change the way the existing attribute is handled, you could do the research and work through that.

If you want to leave the order the same for the two, and add a TODO for latter work on reversing it, I'm also fine.  Note that order doesn't matter if the baseline knowledge is removed as we seem to be working towards.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117921