[PATCH] D105332: [M68k][GloballSel] Lower outgoing return values in IRTranslator

[Min-Yih Hsu via Phabricator via llvm-commits]

myhsu added a comment.

In D105332#2855581 <https://reviews.llvm.org/D105332#2855581>, @gandhi21299 wrote:

> @myhsu x64 debian failure is likely because of the missing M68kISelLowering.h. Can we do something about this?

That's really interesting...clang-format always seems to have a hard time finding that file, I'll look into it



================
Comment at: llvm/lib/Target/M68k/GlSel/M68kCallLowering.cpp:35
+
+  void assignValueToReg(Register ValVReg, Register PhysReg, CCValAssign &VA) {
+    MIB.addUse(PhysReg, RegState::Implicit);
----------------
Missing `override` attribute here, which is a compilation error if you build LLVM using Clang.


================
Comment at: llvm/lib/Target/M68k/GlSel/M68kCallLowering.cpp:42
+  void assignValueToAddress(Register ValVReg, Register Addr, uint64_t Size,
+                            MachinePointerInfo &MPO, CCValAssign &VA) {
+    MachineFunction &MF = MIRBuilder.getMF();
----------------
1. Missing `override`
2. The method you should override is [[ https://github.com/llvm/llvm-project/blob/bf7f846b683077a8adcb229624082e525870229b/llvm/include/llvm/CodeGen/GlobalISel/CallLowering.h#L260 | this one ]]. More specifically, the third argument is mismatched.
3. According to M68k's ABI, we don't use stack for returning, so I don't think we need to implement this method (Put a llvm_unreachable here should be sufficient).


================
Comment at: llvm/lib/Target/M68k/GlSel/M68kCallLowering.cpp:50
+  Register getStackAddress(uint64_t Size, int64_t Offset,
+                           MachinePointerInfo &MPO, ISD::ArgFlagsTy Flags) {
+    llvm_unreachable("unimplemented");
----------------
ditto missing `override`


================
Comment at: llvm/lib/Target/M68k/M68kISelLowering.h:177
 
+  CCAssignFn *getCCAssignFnForReturn(CallingConv::ID CC, bool Return,
+                                     bool IsVarArg) const;
----------------
gandhi21299 wrote:
> Since there is the param `bool Return`, we could combine this function with the above one.
Agree, currently we don't have complicate CC assignment functions so combining them into one is preferred.


================
Comment at: llvm/test/CodeGen/M68k/GlobalISel/irtranslator-ret.ll:185
+  ; CHECK:   [[G_TRUNC1:%[0-9]+]]:_(s8) = G_TRUNC [[G_LOAD1]](s32)
+  ; CHECK:   $bd0 = COPY [[G_TRUNC1]](s8)
+  ; CHECK:   RTS implicit $bd0
----------------
gandhi21299 wrote:
> The name of the register storing the return value, ie $bd0, could be turned into a regex. Please do the same for the tests below.
I don't think so...according to M68k ABI it only uses one (and two in some cases) for return values


================
Comment at: llvm/test/CodeGen/M68k/GlobalISel/irtranslator-ret.ll:213
+  ret i64 %a
+}
----------------
gandhi21299 wrote:
> Floating point tests might be doable? In case its a hastle and Minh does not mind, perhaps add a TODO in the IRTranslator somewhere.
I'm fine with either way. Floating point support is not our main focus now


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

https://reviews.llvm.org/D105332