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

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added a comment.

A few minors - mainly code style concerns, plus a concern about "isM68020()" style function naming.



================
Comment at: llvm/lib/Target/M68k/M68kCollapseMOVEMPass.cpp:1
+//===-- createM68kCollapseMOVEMPass.cpp - Expand MOVEM pass ---*- C++ -*-===//
+//
----------------
M68kCollapseMOVEMPass.cpp ?


================
Comment at: llvm/lib/Target/M68k/M68kFrameLowering.cpp:147
+
+/// findDeadCallerSavedReg - Return a caller-saved register that isn't live
+/// when it reaches the "return" instruction. We can then pop a stack object
----------------
(style) We don't include function names anymore


================
Comment at: llvm/lib/Target/M68k/M68kFrameLowering.cpp:192
+    if (RegMask.PhysReg == Reg)
+      return true;
+  }
----------------
can we use llvm::any_of here?


================
Comment at: llvm/lib/Target/M68k/M68kFrameLowering.cpp:850
+    unsigned FPReg = TRI->getFrameRegister(MF);
+    for (unsigned i = 0; i < CSI.size(); ++i) {
+      if (TRI->regsOverlap(CSI[i].getReg(), FPReg)) {
----------------
(style)
```
for (unsigned i = 0, e = CSI.size(); i < e; ++i) {
```


================
Comment at: llvm/lib/Target/M68k/M68kFrameLowering.cpp:870
+  unsigned Mask = 0;
+  for (size_t i = 0; i < CSI.size(); ++i) {
+    FI = std::max(FI, CSI[i].getFrameIdx());
----------------
(style) use a for-range loop?


================
Comment at: llvm/lib/Target/M68k/M68kFrameLowering.cpp:885
+  const MachineRegisterInfo &RI = MF.getRegInfo();
+  for (size_t i = 0; i < CSI.size(); ++i) {
+    unsigned Reg = CSI[i].getReg();
----------------
(style) use a for-range loop?


================
Comment at: llvm/lib/Target/M68k/M68kFrameLowering.cpp:905
+  unsigned Mask = 0;
+  for (size_t i = 0; i < CSI.size(); ++i) {
+    FI = std::max(FI, CSI[i].getFrameIdx());
----------------
(style) use a for-range loop?


================
Comment at: llvm/lib/Target/M68k/M68kFrameLowering.cpp:917
+  // Append implicit registers and mem locations
+  for (size_t i = 0; i < CSI.size(); ++i) {
+    I.addReg(CSI[i].getReg(), RegState::ImplicitDefine);
----------------
(style) use a for-range loop?


================
Comment at: llvm/lib/Target/M68k/M68kInstrInfo.cpp:366
+
+  auto TRI = getRegisterInfo();
+
----------------
(style) Avoid using auto

Also - should this be a reference?


================
Comment at: llvm/lib/Target/M68k/M68kInstrInfo.cpp:369
+  auto RCDst = TRI.getMaximalPhysRegClass(Dst, MVTDst);
+  auto RCSrc = TRI.getMaximalPhysRegClass(Src, MVTSrc);
+
----------------
(style) Avoid using auto


================
Comment at: llvm/lib/Target/M68k/M68kInstrInfo.cpp:413
+
+  auto TRI = getRegisterInfo();
+
----------------
(style) Avoid using auto


================
Comment at: llvm/lib/Target/M68k/M68kInstrInfo.cpp:416
+  auto RCDst = TRI.getMaximalPhysRegClass(Dst, MVTDst);
+  auto RCSrc = TRI.getMaximalPhysRegClass(Src, MVTSrc);
+
----------------
(style) Avoid using auto


================
Comment at: llvm/lib/Target/M68k/M68kSubtarget.cpp:165
+    return M68kII::MO_GOT;
+  }
+}
----------------
(style)
```
 if (isPositionIndependent())
    return M68kII::MO_GOTPCREL;

  return M68kII::MO_GOT;
```


================
Comment at: llvm/lib/Target/M68k/M68kSubtarget.cpp:197
+      return M68kII::MO_ABSOLUTE_ADDRESS;
+    }
+  }
----------------
(style)
```
  if (isPositionIndependent())
    return M68kII::MO_GOTPCREL;

  if (isM68020())
    return M68kII::MO_PC_RELATIVE_ADDRESS;

  return M68kII::MO_ABSOLUTE_ADDRESS;
```


================
Comment at: llvm/lib/Target/M68k/M68kSubtarget.h:83
+  bool isM68040() const { return SubtargetKind >= M40; }
+  bool isM68060() const { return SubtargetKind >= M60; }
+
----------------
Not sure if the naming here is correct - the function name suggests a specific CPU, but the implementation suggests a minimum CPU feature set.


================
Comment at: llvm/lib/Target/M68k/M68kTargetMachine.cpp:4
-// LLVM<Target>CodeGen need to be present and some APIs
-// like `LLVMInitializeM68kTarget` need to be there
 
----------------
Add the boilerplate to the initial commit?


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

https://reviews.llvm.org/D88391