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

Quentin Colombet via llvm-commits llvm-commits at lists.llvm.org
Thu Apr 7 16:58:28 PDT 2016


qcolombet added a comment.

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


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

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

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

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

================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:713
@@ +712,3 @@
+  return Subtarget.isTargetMachO() &&
+         !Attrs.hasAttrSomewhere(Attribute::SwiftError);
+}
----------------
Shouldn’t we check for swift error support as well?

================
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)
----------------
Shouldn’t this be guard on supportSwiftError or something?

================
Comment at: lib/Target/AArch64/AArch64RegisterInfo.cpp:83
@@ -79,1 +82,3 @@
+  if (MF.getFunction()->getAttributes().hasAttrSomewhere(Attribute::SwiftError))
+    return CSR_AArch64_AAPCS_SwiftError_RegMask;
   if (CC == CallingConv::PreserveMost)
----------------
Ditto.

================
Comment at: lib/Target/ARM/ARMBaseRegisterInfo.cpp:92
@@ +91,3 @@
+      F->getAttributes().hasAttrSomewhere(Attribute::SwiftError))
+    return CSR_iOS_SwiftError_SaveList;
+
----------------
Ditto.

================
Comment at: lib/Target/ARM/ARMBaseRegisterInfo.cpp:120
@@ +119,3 @@
+      MF.getFunction()->getAttributes().hasAttrSomewhere(Attribute::SwiftError))
+    return CSR_iOS_SwiftError_RegMask;
+
----------------
Ditto.

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

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

================
Comment at: lib/Target/X86/X86RegisterInfo.cpp:304
@@ -302,1 +303,3 @@
+        Attribute::SwiftError))
+      return CSR_64_SwiftError_SaveList;
     return CSR_64_SaveList;
----------------
Shouldn’t this be guarded by supportSwiftError()?

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

================
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
----------------
Run opt -instnamer to get rid of the %[0-9]+ variables.

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


http://reviews.llvm.org/D18716





More information about the llvm-commits mailing list