[PATCH] D101819: [M68k][GloballSel] Adding initial GlobalISel infrastructure
Min-Yih Hsu via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Sat Jun 12 10:18:25 PDT 2021
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
More information about the llvm-commits
mailing list