Page MenuHomePhabricator

[Attributor][FIX] Prevent alignment breakage wrt. must-tail calls
ClosedPublic

Authored by jdoerfert on Mar 24 2020, 12:08 AM.

Details

Summary

If we have a must-tail call the callee and caller need to have matching
ABIs. Part of that is alignment which we might modify when we deduce
alignment of arguments of either. Since we would need to keep them in
sync, which is not as simple, we simply avoid deducing alignment for
arguments of the must-tail caller or callee.

Diff Detail

Event Timeline

jdoerfert created this revision.Mar 24 2020, 12:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 24 2020, 12:08 AM

How do we know this is only about alignment?

How do we know this is only about alignment?

Good point. It is about:
Attribute::StructRet, Attribute::ByVal, Attribute::InAlloca, Attribute::InReg, Attribute::Returned, Attribute::SwiftSelf, Attribute::SwiftError, Attribute::Alignment

We derive Returned and Alignment so I need to add the prevention to returned as well. I'll also move it to the manifest as we can use the derived value internally just fine.

jdoerfert updated this revision to Diff 252452.Mar 24 2020, 4:16 PM

Derive but do not manifest alignment and returned for (call site) arguments

Thanks, this looks about reasonable to me, although i'm not familiar with this code to fully review it.

llvm/lib/Transforms/IPO/Attributor.cpp
1364

We are in AAReturnedValues

jdoerfert marked an inline comment as done.Mar 25 2020, 1:09 AM
jdoerfert added inline comments.
llvm/lib/Transforms/IPO/Attributor.cpp
1364

You know, copy&paste ;)

Good point. It is about:
Attribute::StructRet, Attribute::ByVal, Attribute::InAlloca, Attribute::InReg, Attribute::Returned, Attribute::SwiftSelf, Attribute::SwiftError, Attribute::Alignment

Seems reasonable but I'm not sure why we need to care about returned.

jdoerfert added a subscriber: rnk.Mar 25 2020, 7:44 AM

Good point. It is about:
Attribute::StructRet, Attribute::ByVal, Attribute::InAlloca, Attribute::InReg, Attribute::Returned, Attribute::SwiftSelf, Attribute::SwiftError, Attribute::Alignment

Seems reasonable but I'm not sure why we need to care about returned.

It's in the verifier's list that is checked. @rnk can you confirm returned needs to be kept in-sync between caller and callee of a must-tail call?

rnk added a comment.Mar 26 2020, 4:50 PM

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

In 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?

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

rnk added a comment.Mar 27 2020, 1:32 PM
In 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.

In D76673#1946852, @rnk wrote:
In 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?

rnk accepted this revision.Mar 27 2020, 3:38 PM

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

Yes.

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

No, go for it, I forgot to push the button.

This revision is now accepted and ready to land.Mar 27 2020, 3:38 PM
This revision was automatically updated to reflect the committed changes.