[PATCH] D18716: Swift Calling Convention: target-specific changes

[Manman Ren via llvm-commits]

manmanren added a comment.

Hi Quentin,

Thanks for reviewing the patch!

Manman


================
Comment at: lib/Target/AArch64/AArch64CallingConvention.td:284
@@ +283,3 @@
+                                           D8,  D9,  D10, D11,
+                                           D12, D13, D14, D15)>;
+
----------------
qcolombet wrote:
> sub CSR_AArch64_AAPCS, X19?
Yes, will do

================
Comment at: lib/Target/AArch64/AArch64FastISel.cpp:1912
@@ +1911,3 @@
+      return false;
+  }
+
----------------
qcolombet wrote:
> 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.
Will do

================
Comment at: lib/Target/AArch64/AArch64FastISel.cpp:2087
@@ +2086,3 @@
+      return false;
+  }
+
----------------
qcolombet wrote:
> Ditto.
Same here

================
Comment at: lib/Target/AArch64/AArch64FastISel.cpp:3671
@@ +3670,3 @@
+  if (F.getAttributes().hasAttrSomewhere(Attribute::SwiftError) &&
+      TLI.supportSwiftError())
+    return false;
----------------
qcolombet wrote:
> swap the two conditions. hasAttrSomewhere is probably more expensive.
Here too

================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:713
@@ +712,3 @@
+  return Subtarget.isTargetMachO() &&
+         !Attrs.hasAttrSomewhere(Attribute::SwiftError);
+}
----------------
qcolombet wrote:
> Shouldn’t we check for swift error support as well?
I don't see an access to TLI from frame lowering. Should we put it in AArch64FunctionInfo? Any other simpler way?

================
Comment at: lib/Target/AArch64/AArch64RegisterInfo.cpp:56
@@ -54,1 +55,3 @@
+      Attribute::SwiftError))
+    return CSR_AArch64_AAPCS_SwiftError_SaveList;
   if (MF->getFunction()->getCallingConv() == CallingConv::PreserveMost)
----------------
qcolombet wrote:
> Shouldn’t this be guard on supportSwiftError or something?
I don't see an access to TLI from frame lowering. Should we put it in AArch64FunctionInfo? Any other simpler way?

================
Comment at: lib/Target/ARM/ARMFastISel.cpp:1075
@@ -1065,1 +1074,3 @@
+  }
+
   // Verify we have a legal type before going any further.
----------------
Will change this too.

================
Comment at: lib/Target/ARM/ARMFastISel.cpp:1201
@@ -1180,1 +1200,3 @@
+  }
+
   // Verify we have a legal type before going any further.
----------------
Same here

================
Comment at: lib/Target/ARM/ARMFastISel.cpp:2112
@@ +2111,3 @@
+      TLI.supportSwiftError())
+    return false;
+
----------------
Same here

================
Comment at: lib/Target/X86/X86CallingConv.td:854
@@ -847,1 +853,3 @@
 
+def CSR_64_SwiftError : CalleeSavedRegs<(add RBX, R13, R14, R15, RBP)>;
+
----------------
qcolombet wrote:
> sub CSR_64, R12
Yes

================
Comment at: lib/Target/X86/X86FastISel.cpp:984
@@ +983,3 @@
+      return false;
+  }
+
----------------
Will change here too.

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:2359
@@ -2359,2 +2358,3 @@
+    SDValue Val = DAG.getCopyFromReg(RetOps[0], dl, SRetReg,
                                      getPointerTy(MF.getDataLayout()));
 
----------------
qcolombet wrote:
> Unrelated change?
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.

================
Comment at: test/CodeGen/ARM/swifterror.ll:8
@@ +7,3 @@
+
+define float @foo(%swift_error** swifterror %error_ptr_ref) {
+; CHECK-APPLE-LABEL: foo:
----------------
qcolombet wrote:
> Add a comment on what particular configuration this function tests.
will do

================
Comment at: test/CodeGen/ARM/swifterror.ll:27
@@ +26,3 @@
+  store %swift_error* %call.0, %swift_error** %error_ptr_ref
+  %0 = getelementptr inbounds i8, i8* %call, i64 8
+  store i8 1, i8* %0
----------------
qcolombet wrote:
> Run opt -instnamer to get rid of the %[0-9]+ variables.
Yes

================
Comment at: test/CodeGen/ARM/swifterror.ll:32
@@ +31,3 @@
+
+define float @caller(i8* %error_ref) {
+; CHECK-APPLE-LABEL: caller:
----------------
qcolombet wrote:
> Ditto for the comment.
Same here


http://reviews.llvm.org/D18716