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

Fri Mar 27 15:27:09 PDT 2020

jdoerfert added a comment.

In D76673#1946852 <https://reviews.llvm.org/D76673#1946852>, @rnk wrote:

> 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. :)

Fair, but the verifier works on IR and complains about `align` without `byval` ;)
Anyway. I'm fine with not-optimizing this for the moment to appease the verifier for now.

>> 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.

So you think we can relax the verifier check which enforces the `returned` attribute?

---

Long story short, do you have any concerns with this?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76673