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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm 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);
----------------
Using F.getCallingConv() is using the calling function's calling convention. You want Info.CallConv like above


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


================
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
+
----------------
Don't need the 2>&1.

Also, probably should just use update_mir_test_checks


================
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() {
----------------
Value name in declaration list. I'm not sure why even parse this


================
Comment at: llvm/test/CodeGen/M68k/GlobalISel/irtranslator-call.ll:41
+  ret void
+}
----------------
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).


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

https://reviews.llvm.org/D115874