[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/M68kCallingConv.h:62
+
+  bool isPtr = I != CCInfo.F.arg_end() && I->getType()->isPointerTy();
+
----------------
IsPtr


================
Comment at: llvm/lib/Target/M68k/M68kCollapseMOVEMPass.cpp:67
+
+  unsigned getBase() {
+    assert(Base);
----------------
RKSimon wrote:
> constify?
Not done


================
Comment at: llvm/lib/Target/M68k/M68kExpandPseudo.cpp:166
+  case M68k::MOV8cd:
+    return TII->ExpandCCR(MIB, /* isToCCR */ true);
+  case M68k::MOV8dc:
----------------
In a bunch of places. Plus all these kinds of parameters need a capital I.


================
Comment at: llvm/lib/Target/M68k/M68kFrameLowering.cpp:230
+    MachineBasicBlock::iterator I) const {
+  bool reserveCallFrame = hasReservedCallFrame(MF);
+  unsigned Opcode = I->getOpcode();
----------------
Capitalise (pointing out as it's one of the few instances that's not of the form `bool isX`)


================
Comment at: llvm/lib/Target/M68k/M68kFrameLowering.cpp:350
+      // load the offset into a register and do one sub/add
+      unsigned Reg = 0;
+
----------------
`Register`?


================
Comment at: llvm/lib/Target/M68k/M68kFrameLowering.cpp:353
+      if (isSub && !isRegLiveIn(MBB, M68k::D0))
+        Reg = (unsigned)(M68k::D0);
+      else
----------------
Unnecessary parens


================
Comment at: llvm/lib/Target/M68k/M68kFrameLowering.cpp:386
+                                      MachineBasicBlock::iterator &MBBI,
+                                      bool doMergeWithPrevious) const {
+  if ((doMergeWithPrevious && MBBI == MBB.begin()) ||
----------------
DoMergeWithPrevious, or better just MergeWithPrevious, the Do is superfluous.


================
Comment at: llvm/lib/Target/M68k/M68kFrameLowering.h:175
+  /// of uses and size in order to minimize code size.
+  // void orderFrameObjects(const MachineFunction &MF,
+  //                        SmallVectorImpl<int> &ObjectsToAllocate) const
----------------
Commented-out code. TODO probably belongs in the implementation not the header file, too, even if it does relate to overriding a specific hook.


================
Comment at: llvm/lib/Target/M68k/M68kISelLowering.cpp:2243
+SDValue M68kTargetLowering::LowerBRCOND(SDValue Op, SelectionDAG &DAG) const {
+  bool addTest = true;
+  SDValue Chain = Op.getOperand(0);
----------------
Capitalise


================
Comment at: llvm/lib/Target/M68k/M68kISelLowering.cpp:2756
+  // true/false values to select between, and a branch opcode to use.
+  const BasicBlock *LLVM_BB = BB->getBasicBlock();
+  MachineFunction::iterator It = ++BB->getIterator();
----------------
Style, but also bad name (everything's LLVM, what you mean is that it's an IR BB not a Machine BB); BB vs MBB is the usual way to distinguish between the two.


================
Comment at: llvm/lib/Target/M68k/M68kISelLowering.cpp:2765
+  //   fallthrough --> copy0MBB
+  MachineBasicBlock *thisMBB = BB;
+  MachineFunction *F = BB->getParent();
----------------
Capitalise all the names of the MBB variables here.


================
Comment at: llvm/lib/Target/M68k/M68kISelLowering.cpp:3133-3138
+      while (
+          Carry.getOpcode() == ISD::TRUNCATE ||
+          Carry.getOpcode() == ISD::ZERO_EXTEND ||
+          Carry.getOpcode() == ISD::SIGN_EXTEND ||
+          Carry.getOpcode() == ISD::ANY_EXTEND ||
+          (Carry.getOpcode() == ISD::AND && isOneConstant(Carry.getOperand(1))))
----------------
unless clang-format disagrees in its infinite wisdom


================
Comment at: llvm/lib/Target/M68k/M68kISelLowering.h:186
+                           const CCValAssign &VA, MachineFrameInfo &MFI,
+                           unsigned i) const;
+
----------------
Use a better name (and capitalise)


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

https://reviews.llvm.org/D88391