[PATCH] D76673: [Attributor][FIX] Prevent alignment breakage wrt. must-tail calls

Fri Mar 27 13:43:19 PDT 2020

rnk added a comment.

In D76673#1945275 <https://reviews.llvm.org/D76673#1945275>, @jdoerfert wrote:

> In D76673#1945260 <https://reviews.llvm.org/D76673#1945260>, @rnk wrote:
>
> > lgtm
> >
> > I think the current verifier check is overly conservative. I think the `align` argument attribute is only an ABI-relevant attribute when it is used with `byval`. In your test case, it seems to refer to the alignment of the pointee of a pointer argument, not the alignment of the argument itself. In that case, it is not ABI relevant, and we could tolerate the mismatch.
> >
> > However, I just don't think it's worth worrying about this detail in attributor. Mainly, it points out that `align` is overloaded. =(
>
>
> I don't think in my test it is the pointee of the pointer argument. The load from %p has an alignment of 32, so %p is assumed to be align 32 at this location. Then the argument of `musttail_callee_1` is assumed to be align 32 as well but we do not manifest it.
>  What am I getting wrong?

Sorry, I'm not being clear. Suppose an argument is passed in memory. What is the alignment of that memory? When the attributes `byval(<ty>) align N` are used, it indicates the alignment of the argument memory.

>From the perspective of LLVM IR, byval arguments are represented with a pointer SSA value, so the align attribute means almost the same thing: it means some number of low bits of this pointer are zero.

But from the perspective of call lowering, the align attribute can either be a statement about the bits of the argument value, or a statement about how to lay out arguments in memory on the stack, depending on the presence of other attributes. I tend to see things from this perspective, not the perspective of an IR optimizer. :)

> Also, the verifier check lists `returned` as well. Is it correct that `returned` has ABI implications?

I think it's just an optimization hint. I believe the IR has to actually return the value in question, so it would be semantics-preserving to discard all `returned` attributes. I could be wrong, though.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76673