[PATCH] D104542: [M68k][GloballSel] Lower i32, i32 values

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

myhsu added inline comments.


================
Comment at: llvm/lib/Target/M68k/GlSel/M68kCallLowering.cpp:82
+                                                CCValAssign &VA) {
+  assert(VA.isRegLoc() && "Value shouldn't be assigned to reg");
+  assert(VA.getLocReg() == PhysReg && "Assigning to the wrong reg?");
----------------
I think you're trying to say either "Value should be assigned to reg" or "Value was not assigned to reg?" right?


================
Comment at: llvm/lib/Target/M68k/GlSel/M68kCallLowering.cpp:97
+                                                    CCValAssign &VA) {
+  llvm_unreachable("Don't know how to assign a value to an address yet");
+}
----------------
You can just put "unimeplemented" here. Since the stack trace will tell you which function triggers this unreachable statement


================
Comment at: llvm/lib/Target/M68k/GlSel/M68kCallLowering.cpp:104
+                                                   ISD::ArgFlagsTy Flags) {
+  llvm_unreachable("Don't know how to get a stack address yet");
 }
----------------
ditto


================
Comment at: llvm/lib/Target/M68k/GlSel/M68kCallLowering.h:47
 
+class M68kIncomingValueHandler : public CallLowering::IncomingValueHandler {
+public:
----------------
(nit) please use struct instead and remove "public" in the following line


================
Comment at: llvm/lib/Target/M68k/M68kISelLowering.h:174
 
+  CCAssignFn *ccAssignFnForCall(CallingConv::ID CC, bool Return,
+                                bool IsVarArg) const;
----------------
The function name is a little confusing, maybe something like "getCCAssignGnForCall"?


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

https://reviews.llvm.org/D104542