[PATCH] D15341: Separate CSRs into a subset handled via explicit copies and a subset handled by prologue/epilogue (AArch64)

[Quentin Colombet via llvm-commits]

qcolombet added a comment.

Hi Manman,

Thanks for your patience.

Here are a few comments.

Cheers,
-Quentin


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:3286
@@ +3285,3 @@
+      if (AArch64::GPR64RegClass.contains(*I)) {
+        RetOps.push_back(DAG.getRegister(*I, getPointerTy(DAG.getDataLayout())));
+      }
----------------
Why is this using a pointer type instead of an i64?


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:3287
@@ +3286,3 @@
+        RetOps.push_back(DAG.getRegister(*I, getPointerTy(DAG.getDataLayout())));
+      }
+      else if (AArch64::FPR64RegClass.contains(*I)) {
----------------
No need for {}

================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:3290
@@ +3289,3 @@
+        RetOps.push_back(DAG.getRegister(*I, MVT::getFloatingPointVT(64)));
+      }
+      else
----------------
Ditto.

================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:3293
@@ +3292,3 @@
+        llvm_unreachable("Unexpected register class in CSRsViaCopy!");
+    }
+  }
----------------
Was this clang-formated?
The layout looks funny to me.

================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:10069
@@ +10068,3 @@
+    // CFI pseudo-instructions.
+    Entry->addLiveIn(*I);
+    BuildMI(*Entry, Entry->begin(), DebugLoc(),
----------------
Could you assert that the MachineFunction have the nounwind attribute?

================
Comment at: lib/Target/AArch64/AArch64ISelLowering.h:392
@@ +391,3 @@
+  bool supportSplitCSR(CallingConv::ID CC) const override {
+    return CC == CallingConv::CXX_FAST_TLS;
+  }
----------------
I think we should change the hook to take a MachineFunction.
That way we could check that the function does have the nounwind attribute and if it does not, we can say this is not supported.

================
Comment at: lib/Target/AArch64/AArch64MachineFunctionInfo.h:75
@@ -74,1 +74,3 @@
 
+  bool IsSplitCSR;
+
----------------
Add a comment for that field.

================
Comment at: lib/Target/AArch64/AArch64RegisterInfo.cpp:61
@@ +60,3 @@
+  assert(MF && "Invalid MachineFunction pointer.");
+  if (MF->getFunction()->getCallingConv() == CallingConv::CXX_FAST_TLS &&
+      MF->getInfo<AArch64FunctionInfo>()->isSplitCSR())
----------------
Isn’t this redundant with the isSplitCSR check?
I mean, if we want to support more calling convention, we should minimize the places we have to patch to do so.

================
Comment at: test/CodeGen/AArch64/cxx-tlscc.ll:50
@@ -52,1 +49,3 @@
+; CHECK-NOT: stp x4, x3
+; CHECK-NOT: stp x2, x1
 ; CHECK: blr
----------------
Nice! :)


http://reviews.llvm.org/D15341