[PATCH] D29599: Clang Changes for alloc_align

[Erich Keane via Phabricator via cfe-commits]

erichkeane marked 2 inline comments as done.
erichkeane added inline comments.


================
Comment at: include/clang/Basic/AttrDocs.td:252
+declaration to specify that the return value of the function (which must be a
+pointer type) has an alignment specified by the indicated parameter. The
+alignment parameter is one-based. In the case of member functions, the
----------------
rjmccall wrote:
> "that the return value of the function ... is at least as aligned as the
> value of the indicated parameter.  The parameter is given by its index
> in the list of formal parameters; the first parameter has index 1 unless
> the function is a C++ non-static member function, in which case the
> first parameter has index 2 to account for the implicit ``this`` parameter."
> 
> What's the behavior of this attribute if the given attribute is not a power of 2?
> Does it silently have no effect, or is it U.B.?
Thanks for the wording, I like this.

It isn't checked for that, so I'll say UB.  A little bit of a caveat emptor here.


================
Comment at: include/clang/Basic/AttrDocs.td:270
+condition that the code already ensures is true. It does not cause the compiler
+to enforce the provided alignment assumption.
+  }];
----------------
rjmccall wrote:
> "Note that this attribute merely informs the compiler that a function always
> returns a sufficiently aligned pointer.  It does not cause the compiler to emit
> code to enforce that alignment.  The behavior is undefined if the returned
> pointer is not sufficiently aligned."
Thanks again!


================
Comment at: lib/CodeGen/CGCall.cpp:4352
+    } else if (const auto *AA = TargetDecl->getAttr<AllocAlignAttr>()) {
+      llvm::Value *ParamVal = IRCallArgs[AA->getParamIndex() - 1];
+      EmitAlignmentAssumption(Ret.getScalarVal(), ParamVal);
----------------
rjmccall wrote:
> IRCallArgs is not one-to-one with the list of formal parameters.  You need to be taking the value out of CallArgList.  Three test cases come to mind:
> 
> 1. There's an earlier struct argument which expands to no IR arguments.
> 2. There's an earlier struct argument which expands to multiple IR arguments.
> 3. The alignment argument itself expands to multiple IR arguments.  For example, __int128_t on x86-64.
Awesome, thanks for the info!  I'll change that.


https://reviews.llvm.org/D29599