[PATCH] D36331: Add ARC backend

[Krzysztof Parzyszek via Phabricator via llvm-commits]

kparzysz added inline comments.


================
Comment at: llvm/lib/Target/ARC/ARCFrameLowering.cpp:49
+                                    const ARCInstrInfo &TII, DebugLoc dl,
+                                    int amount, int StackPtr) {
+  unsigned adjop;
----------------
Variable names typically start with a capital letter.  The 'dl' is kind of an exception (since DL usually refers to DataLayout), but then you have "amount" (starting with lowercase) and immediately after that "StackPtr", which is capitalized.


================
Comment at: llvm/lib/Target/ARC/ARCFrameLowering.cpp:78
+static unsigned
+DetermineLastCalleeSave(const std::vector<CalleeSavedInfo> &CSI) {
+  unsigned last = 0;
----------------
Function names, on the other names usually start with a lowercase.


================
Comment at: llvm/lib/Target/ARC/ARCFrameLowering.cpp:114
+
+/// emitProlog/emitEpilog - These methods insert prolog and epilog code into
+/// the function.
----------------
The current guidelines are to omit the function names from the comments.


================
Comment at: llvm/lib/Target/ARC/ARCFrameLowering.cpp:124
+  const auto *TII =
+      static_cast<const ARCInstrInfo *>(MF.getSubtarget().getInstrInfo());
+  MachineBasicBlock::iterator MBBI = MBB.begin();
----------------
FYI, you can get the subtarget for your target via MF.getSubtarget<ARCSubtarget>().  This could avoid doing the static_cast, assuming that ARCSubtarget returns ARCInstrInfo.


================
Comment at: llvm/lib/Target/ARC/ARCFrameLowering.cpp:156
+    // BL to __save_r13_to_<TRI->getRegAsmName()>
+    const auto *TII = static_cast<const ARCInstrInfo *>(
+        MBB.getParent()->getSubtarget().getInstrInfo());
----------------
You already have TII.


================
Comment at: llvm/lib/Target/ARC/ARCFrameLowering.cpp:277
+  if (StackSize - AmountAboveFunclet) {
+    BuildMI(MBB, MBBI, MBBI->getDebugLoc(), TII->get(ARC::ADD_rru6))
+        .addReg(ARC::SP)
----------------
MBBI could be end(). It's better to use MBB.findDebugLoc(MBBI) instead.


================
Comment at: llvm/lib/Target/ARC/ARCFrameLowering.cpp:284
+  if (StackSlotsUsedByFunclet) {
+    const auto *TII = static_cast<const ARCInstrInfo *>(
+        MBB.getParent()->getSubtarget().getInstrInfo());
----------------
Duplicate TII again.


================
Comment at: llvm/lib/Target/ARC/ARCISelDAGToDAG.cpp:108
+  if (ConstantSDNode *RHS = dyn_cast<ConstantSDNode>(Addr.getOperand(1))) {
+    auto RHSC = (int32_t)RHS->getSExtValue();
+    if (Addr.getOpcode() == ISD::SUB)
----------------
This is `int32_t RHSC = RHS->getSExtValue()`, the `auto` makes it look strange.  In general we favor explicit type, unless the type is obvious and/or using auto saves space.


================
Comment at: llvm/lib/Target/ARC/ARCISelLowering.cpp:341
+
+  for (unsigned I = 0, E = RegsToPass.size(); I != E; ++I)
+    Ops.push_back(DAG.getRegister(RegsToPass[I].first,
----------------
This loop uses uppercase controlling variables, while the ones above use lowercase.  Please make it consistent.


================
Comment at: llvm/lib/Target/ARC/ARCRegisterInfo.cpp:40
+
+bool ARCRegisterInfo::isGPR32Register(unsigned Reg) {
+  switch (Reg) {
----------------
This function is equivalent to `ARC::GPR32RegClass.contains(Reg)`.


================
Comment at: llvm/lib/Target/ARC/ARCRegisterInfo.cpp:224
+  DEBUG(dbgs() << "<--------->\n");
+  DEBUG(MI.print(dbgs()));
+  DEBUG(dbgs() << "FrameIndex         : " << FrameIndex << "\n");
----------------
FYI, MI can be dumped directly to dbgs(), e.g. dbgs() << MI.


================
Comment at: llvm/lib/Target/ARC/ARCSubtarget.h:58
+  }
+  const TargetRegisterInfo *getRegisterInfo() const override {
+    return &InstrInfo.getRegisterInfo();
----------------
This should return `const ARCRegisterInfo*`.


https://reviews.llvm.org/D36331