[PATCH] D88391: [M68k] (Patch 5/8) Target lowering

[Jessica Clarke via Phabricator via llvm-commits]

jrtc27 added inline comments.


================
Comment at: llvm/lib/Target/M68k/M68kISelDAGToDAG.cpp:178
+  StringRef getPassName() const override {
+    return "M680X0 DAG->DAG Pattern Instruction Selection";
+  }
----------------
Old name still left


================
Comment at: llvm/lib/Target/M68k/M68kISelLowering.h:35
+
+  CALL,
+  RET,
----------------
` = FIRST_NUMBER`?


================
Comment at: llvm/lib/Target/M68k/M68kISelLowering.h:82
+
+  GlobalBaseReg,
+
----------------
Change of naming style


================
Comment at: llvm/lib/Target/M68k/M68kISelLowering.h:95
+  // falls back to heap allocation if not.
+  SEG_ALLOCA,
+};
----------------
Corresponding `LAST_NUMBER`?


================
Comment at: llvm/lib/Target/M68k/M68kInstrBuilder.h:40
+namespace llvm {
+
+static inline const MachineInstrBuilder &
----------------
`namespace M68k`?


================
Comment at: llvm/lib/Target/M68k/M68kInstrInfo.cpp:835
+  static const std::pair<unsigned, const char *> TargetFlags[] = {
+      {MO_ABSOLUTE_ADDRESS, "M68k-absolute"},
+      {MO_PC_RELATIVE_ADDRESS, "M68k-pcrel"},
----------------
These are conventionally all lowercase (otherwise GOT etc would be uppercase too)


================
Comment at: llvm/lib/Target/M68k/M68kRegisterInfo.cpp:155-160
+  // // Set the global-base-pointer register and its aliases as reserved if
+  // needed. if (getGlobalBaseRegister()) {
+  //   for (MCSubRegIterator I(getGlobalBaseRegister(), this,
+  //   #<{(|Self=|)}>#true); I.isValid(); ++I)
+  //     Reserved.set(*I);
+  // }
----------------
?


================
Comment at: llvm/lib/Target/M68k/M68kRegisterInfo.cpp:213-218
+  // if (Size == 16) {
+  //     assert(isInt<16>(Offset) && "Cannot use disp greater 16 bit");
+  // } else if (Size == 8) {
+  //     assert(isInt<8>(Offset) && "Cannot use disp greater 8 bit");
+  // } else {
+  // }
----------------
?


================
Comment at: llvm/lib/Target/M68k/M68kRegisterInfo.cpp:276-280
+  // if (isInt<8>(size)) {
+  //     return &M68k::DR8RegClass;
+  // } else if (isInt<16>(size)) {
+  //     return &M68k::DR16RegClass;
+  // }
----------------
?


================
Comment at: llvm/lib/Target/M68k/M68kSubtarget.cpp:231
+  // If the function is marked as non-lazy, generate an indirect call
+  // which loads from the GOT directly. This avoids runtime overhead
+  // at the cost of eager binding.
----------------



================
Comment at: llvm/lib/Target/M68k/M68kTargetMachine.cpp:47-59
+  // M68k pointers are always 32 bit wide even for 16 bit cpus
+  Ret += "-p:32:32";
+
+  // M68k requires i8 to align on 2 byte boundry
+  Ret += "-i8:8:8-i16:16:16-i32:32:32";
+
+  // FIXME #29 no floats at the moment
----------------
As with clang re alignment.


================
Comment at: llvm/lib/Target/M68k/M68kTargetObjectFile.cpp:42
+
+  // FIXME #32 do i need them explicitly?
+  SmallDataSection = getContext().getELFSection(
----------------
Need what?


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

https://reviews.llvm.org/D88391