[PATCH] D115874: [M68k][GlobalISel] Implement lowerCall based on M68k calling convention

[Sheng via Phabricator via llvm-commits]

0x59616e added inline comments.


================
Comment at: llvm/lib/Target/M68k/GISel/M68kCallLowering.cpp:199
+  CCAssignFn *AssignFn =
+      TLI.getCCAssignFn(F.getCallingConv(), false, F.isVarArg());
+  OutgoingValueAssigner Assigner(AssignFn);
----------------
arsenm wrote:
> Using F.getCallingConv() is using the calling function's calling convention. You want Info.CallConv like above
Done. Thanks :)


================
Comment at: llvm/lib/Target/M68k/GISel/M68kCallLowering.cpp:215
+    CCAssignFn *RetAssignFn =
+        TLI.getCCAssignFn(F.getCallingConv(), true, F.isVarArg());
+
----------------
arsenm wrote:
> Ditto
Done. Thanks :)


================
Comment at: llvm/test/CodeGen/M68k/GlobalISel/irtranslator-call.ll:1
+; RUN: llc -mtriple=m68k -O0 -global-isel -stop-after=irtranslator -verify-machineinstrs %s -o - 2>&1 | FileCheck %s
+
----------------
arsenm wrote:
> Don't need the 2>&1.
> 
> Also, probably should just use update_mir_test_checks
Done. Thanks :)


================
Comment at: llvm/test/CodeGen/M68k/GlobalISel/irtranslator-call.ll:37
+; CHECK: RTS
+declare void @simple_arg_callee(i32 %0)
+define void @test_simple_arg() {
----------------
arsenm wrote:
> Value name in declaration list. I'm not sure why even parse this
I added it accidentally. It should not have been there. I've removed it. Thanks :)


================
Comment at: llvm/test/CodeGen/M68k/GlobalISel/irtranslator-call.ll:41
+  ret void
+}
----------------
arsenm wrote:
> These tests aren't comprehensive enough. You should add a wider variety of return value and outgoing argument types (particularly some pointers and aggregates). Maybe add some byval / sret too.
> 
> You're also testing a few different call instruction types which aren't covered (in particular indirect call and the isPositionIndependent switch).
I've added a few more tests. Thanks for reviewing ;)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115874/new/

https://reviews.llvm.org/D115874