[PATCH] D34602: [GlobalISel][X86] Support call ABI.

[Oren Ben Simhon via Phabricator via llvm-commits]

oren_ben_simhon added inline comments.


================
Comment at: lib/Target/X86/X86CallLowering.cpp:300
+  SmallVector<ArgInfo, 8> SplitArgs;
+  for (auto &OrigArg : OrigArgs) {
+    splitToValueTypes(OrigArg, SplitArgs, DL, MRI,
----------------
Since you do not modify OrigArg you can define it as const &


================
Comment at: lib/Target/X86/X86CallLowering.cpp:317
+  // constraint of that instruction.
+  if (Callee.isReg())
+    MIB->getOperand(0).setReg(constrainOperandRegClass(
----------------
Could you please add a test that checks the case where the callee is a register?


================
Comment at: lib/Target/X86/X86CallLowering.cpp:324
+  // Finally we can copy the returned value back into its virtual-register. In
+  // symmetry with the arugments, the physical register must be an
+  // implicit-define of the call instruction.
----------------
typo arguments


================
Comment at: lib/Target/X86/X86CallLowering.cpp:327
+
+  if (OrigRet.Reg) {
+    SplitArgs.clear();
----------------
Please consider having a separate method for handling call results.


================
Comment at: lib/Target/X86/X86CallLowering.cpp:344
+
+  CallSeqStart.addImm(Handler.StackSize).addImm(0).addImm(0);
+
----------------
Could you please document the immediate additions?


================
Comment at: test/CodeGen/X86/GlobalISel/callingconv.ll:123
+
+declare void @trivial_callee()
+define void @test_trivial_call() {
----------------
Could you please add tests that exercise arguments and return values that are spitted (like structures)?


https://reviews.llvm.org/D34602