[PATCH] D101819: [M68k][GloballSel] Adding initial GlobalISel infrastructure

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

myhsu added a comment.

First of all, big thanks for doing this :-)
Sorry for coming in late. This patch LGTM in general, just have some formatting / coding style comments.



================
Comment at: llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp:3132
     R << "unable to translate in big endian mode";
+
     reportTranslationError(*MF, *TPC, *ORE, R);
----------------
(nit) please remove this empty line if it's not necessary


================
Comment at: llvm/lib/Target/M68k/GlSel/M68kCallLowering.cpp:29
+
+  if (Val != nullptr)
+    return false;
----------------



================
Comment at: llvm/lib/Target/M68k/GlSel/M68kCallLowering.h:10
+/// \file
+/// This file describes how to lower LLVM calls to machine code calls.
+//
----------------
Can you use the same file-level comment as M68kCallLowering.cpp?


================
Comment at: llvm/lib/Target/M68k/GlSel/M68kCallLowering.h:25
+
+class M68kCallLowering : public CallLowering {
+
----------------
Can you also a TODO here (or in the implementation side) to note that we're only supporting return instruction with no value at this time point


================
Comment at: llvm/lib/Target/M68k/GlSel/M68kInstructionSelector.cpp:16
+
+#define DEBUG_TYPE "M68k-isel"
+
----------------



================
Comment at: llvm/lib/Target/M68k/GlSel/M68kLegalizerInfo.h:9
+/// \file
+/// This file declares the targeting of the Machinelegalizer class for M68k.
+//===----------------------------------------------------------------------===//
----------------



================
Comment at: llvm/lib/Target/M68k/GlSel/M68kLegalizerInfo.h:22
+/// This class provides the information for the target register banks.
+class M68kLegalizerInfo : public LegalizerInfo {
+public:
----------------
(nit) Can you use struct instead of class since there is not private member here?


================
Comment at: llvm/lib/Target/M68k/GlSel/M68kRegisterBankInfo.cpp:22
+#include "M68kGenRegisterBank.inc"
+
+using namespace llvm;
----------------
ditto, missing `#undef`?


================
Comment at: llvm/lib/Target/M68k/GlSel/M68kRegisterBankInfo.h:19
+#define GET_REGBANK_DECLARATIONS
+#include "M68kGenRegisterBank.inc"
+
----------------
should there be a `#undef GET_REGBANK_DECLARATIONS`? right after this line?


================
Comment at: llvm/lib/Target/M68k/GlSel/M68kRegisterBankInfo.h:29
+#include "M68kGenRegisterBank.inc"
+};
+
----------------
ditto, should there be a `#undef` here?


================
Comment at: llvm/lib/Target/M68k/GlSel/M68kRegisterBanks.td:8
+//===----------------------------------------------------------------------===//
+//
+//
----------------
Can you add a brief file-level comment?


================
Comment at: llvm/lib/Target/M68k/M68kSubtarget.cpp:64
+  InstSelector.reset(createM68kInstructionSelector(
+      *static_cast<const M68kTargetMachine *>(&TM), *this, *RBI));
+}
----------------
this casting seems to be redundant


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

https://reviews.llvm.org/D101819