This is an archive of the discontinued LLVM Phabricator instance.

Swift Calling Convention: target-specific changes
ClosedPublic

Authored by manmanren on Apr 1 2016, 3:43 PM.

Details

Summary

This patch applies on top of (reviews.llvm.org/D18108). This is the last patch for swifterror, the first patch was committed as r265189.

Diff Detail

Repository
rL LLVM

Event Timeline

manmanren updated this revision to Diff 52437.Apr 1 2016, 3:43 PM
manmanren retitled this revision from to Swift Calling Convention: target-specific changes.
manmanren updated this object.
manmanren added reviewers: hfinkel, qcolombet.
manmanren added subscribers: rjmccall, llvm-commits.

Per review comment in reviews.llvm.org/D18108), I will add a testing case: multiple swifterror values exist in the caller and the actual argument is not the first entry in SwiftErrorVals.

manmanren updated this revision to Diff 52816.Apr 6 2016, 9:45 AM

Adding a testing case with multiple swifterror values, this testing case will fail if we always use the first entry in SwiftErrorVals in LowerCallTo.

qcolombet edited edge metadata.Apr 7 2016, 4:58 PM

Hi Manman,

The functionality seems mostly good to me. I have highlighted on the first target the places that need the same kind of fixes you did for the target independent patch. You need to apply this to the three targets.

Regarding the tests, please add a few comments on what each function tests and also run opt -instnamer.

Thanks,
-Quentin

lib/Target/AArch64/AArch64CallingConvention.td
284 ↗(On Diff #52816)

sub CSR_AArch64_AAPCS, X19?

lib/Target/AArch64/AArch64FastISel.cpp
1912 ↗(On Diff #52816)

Same comment as the generic code of the previous review: Guard the whole thing with supportSwiftError and add a comment why we need to check these two.

2087 ↗(On Diff #52816)

Ditto.

3671 ↗(On Diff #52816)

swap the two conditions. hasAttrSomewhere is probably more expensive.

lib/Target/AArch64/AArch64FrameLowering.cpp
713 ↗(On Diff #52816)

Shouldn’t we check for swift error support as well?

lib/Target/AArch64/AArch64RegisterInfo.cpp
56 ↗(On Diff #52816)

Shouldn’t this be guard on supportSwiftError or something?

83 ↗(On Diff #52816)

Ditto.

lib/Target/ARM/ARMBaseRegisterInfo.cpp
92 ↗(On Diff #52816)

Ditto.

120 ↗(On Diff #52816)

Ditto.

lib/Target/X86/X86CallingConv.td
854 ↗(On Diff #52816)

sub CSR_64, R12

lib/Target/X86/X86ISelLowering.cpp
2359 ↗(On Diff #52816)

Unrelated change?

lib/Target/X86/X86RegisterInfo.cpp
304 ↗(On Diff #52816)

Shouldn’t this be guarded by supportSwiftError()?

test/CodeGen/ARM/swifterror.ll
8 ↗(On Diff #52816)

Add a comment on what particular configuration this function tests.

27 ↗(On Diff #52816)

Run opt -instnamer to get rid of the %[0-9]+ variables.

32 ↗(On Diff #52816)

Ditto for the comment.

Hi Quentin,

Thanks for reviewing the patch!

Manman

lib/Target/AArch64/AArch64CallingConvention.td
284 ↗(On Diff #52816)

Yes, will do

lib/Target/AArch64/AArch64FastISel.cpp
1912 ↗(On Diff #52816)

Will do

2087 ↗(On Diff #52816)

Same here

3671 ↗(On Diff #52816)

Here too

lib/Target/AArch64/AArch64FrameLowering.cpp
713 ↗(On Diff #52816)

I don't see an access to TLI from frame lowering. Should we put it in AArch64FunctionInfo? Any other simpler way?

lib/Target/AArch64/AArch64RegisterInfo.cpp
56 ↗(On Diff #52816)

I don't see an access to TLI from frame lowering. Should we put it in AArch64FunctionInfo? Any other simpler way?

lib/Target/ARM/ARMFastISel.cpp
1075 ↗(On Diff #52816)

Will change this too.

1201 ↗(On Diff #52816)

Same here

2112 ↗(On Diff #52816)

Same here

lib/Target/X86/X86CallingConv.td
854 ↗(On Diff #52816)

Yes

lib/Target/X86/X86FastISel.cpp
984 ↗(On Diff #52816)

Will change here too.

lib/Target/X86/X86ISelLowering.cpp
2359 ↗(On Diff #52816)

This is actually needed for the case of swifterror with sret. The testing case will fail without this change.

More info: We need to change the input chain for "CopyFromReg" of sret. It should have no effect when swifterror is not used, since the input chain should be the same with and without this change.

When sret is used with swifterror, we create a few SDNodes: "CopyToReg" of swifterror, "CopyFromReg" of sret, "CopyToReg" of sret. Before this change, the input chain for "CopyFromReg" of sret comes from "CopyToReg" of swifterror, and we will have a cycle in Unit graph. "CopyToReg" of swifterror and sret will be glued together into a single unit (Unit A), "CopyFromReg" of sret will be in a different unit (Unit B). We will have cyclic dependency between Unit A and Unit B:
Data dependency from Unit B to Unit A due to "CopyToReg" of sret using data from "CopyFromReg" of sret
Chain dependency from Unit A to Unit B due to "CopyFromReg" of sret using chain from "CopyToReg" of swifterror

This change uses the chain prior to handling the result values, as input chain for "CopyFromReg" of sret, to remove the cyclic dependency.

I will update the comment.

test/CodeGen/ARM/swifterror.ll
8 ↗(On Diff #52816)

will do

27 ↗(On Diff #52816)

Yes

32 ↗(On Diff #52816)

Same here

manmanren updated this revision to Diff 53273.Apr 11 2016, 10:22 AM
manmanren edited edge metadata.

Addressing review comments.

manmanren marked 13 inline comments as done.Apr 11 2016, 10:28 AM

The part that is not addressed is using the flag "supportSwiftError" from RegisterInfo or FrameLowering. I am not sure the best way to do this.
Currently supportSwiftError is in TLI, and FrameLowering and RegisterInfo do not have access to TLI, I think.

MachineFunctionInfo doesn't look like the right place to add this, since this is not specific to each function.

Thanks for reviewing, Quentin!

Manman

Hi Manman,

I haven’t checked all the places, but I think you should have access to TLI through the MachineFunction then the sub target everywhere.

Cheers,
-Quentin

lib/Target/AArch64/AArch64FrameLowering.cpp
713 ↗(On Diff #53273)

You should have access to it through the sub target you get from the function.

lib/Target/AArch64/AArch64RegisterInfo.cpp
56 ↗(On Diff #53273)

MF->getSubtarget()->getTargetLowering().

lib/Target/X86/X86ISelLowering.cpp
2304 ↗(On Diff #53273)

The comment helps :).

manmanren updated this revision to Diff 53302.Apr 11 2016, 12:06 PM

Addressing comments

manmanren marked 6 inline comments as done.Apr 11 2016, 12:08 PM

Cheers,
Manman

lib/Target/AArch64/AArch64FrameLowering.cpp
713 ↗(On Diff #53302)

Yes, you are right.

qcolombet accepted this revision.Apr 11 2016, 1:22 PM
qcolombet edited edge metadata.
qcolombet added inline comments.
test/CodeGen/AArch64/swifterror.ll
8 ↗(On Diff #53302)

tests -> test

36 ↗(On Diff #53302)

Could you add a description for the test?

249 ↗(On Diff #53302)

Missing description.

339 ↗(On Diff #53302)

Description.

This revision is now accepted and ready to land.Apr 11 2016, 1:22 PM
This revision was automatically updated to reflect the committed changes.
MatzeB added a subscriber: MatzeB.Apr 11 2016, 9:02 PM
MatzeB added inline comments.
llvm/trunk/lib/Target/AArch64/AArch64RegisterInfo.cpp
84–87

In case of a call the MachineFunction passed to getCallPreservedMask() appears to be the callers machine function (because in general we do not have a machine function for the callee), but I believe this check should be about the callee...