[PATCH] D145658: [Xtensa] Initial support of the ALU operations.

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/Xtensa/XtensaCallingConv.td:20
+
+  //First two return values go in a2, a3, a4, a5
+  CCIfType<[i32], CCAssignToReg<[A2, A3, A4, A5]>>,
----------------
Missing space after //


================
Comment at: llvm/lib/Target/Xtensa/XtensaISelDAGToDAG.cpp:45
+    report_fatal_error("MemReg address is not implemented yet");
+    return true;
+  }
----------------
remove dead return


================
Comment at: llvm/lib/Target/Xtensa/XtensaISelDAGToDAG.cpp:77
+  if (Node->isMachineOpcode()) {
+    LLVM_DEBUG(dbgs() << "== "; Node->dump(CurDAG); dbgs() << "\n");
+    Node->setNodeId(-1);
----------------
Yes, this is redundant printing


================
Comment at: llvm/lib/Target/Xtensa/XtensaISelLowering.cpp:69-70
+    State.addLoc(CCValAssign::getMem(ValNo, ValVT, Offset, LocVT, LocInfo));
+    // Allocate rest of registers, because rest part is not used to pass
+    // arguments
+    while (State.AllocateReg(IntRegs)) {
----------------
Grammar needs work on this comment


================
Comment at: llvm/lib/Target/Xtensa/XtensaISelLowering.cpp:71-72
+    // arguments
+    while (State.AllocateReg(IntRegs)) {
+    }
+    return false;
----------------
Why doesn't it work to just leave them unallocated?


================
Comment at: llvm/lib/Target/Xtensa/XtensaISelLowering.cpp:157
+  if (IsVarArg) {
+    llvm_unreachable("Var arg not supported by FormalArguments Lowering");
+  }
----------------
Use DiagnosticInfoUnsupported and return undefs (or at least report_fatal_error) for unsupported things instead of unreachable


================
Comment at: llvm/lib/Target/Xtensa/XtensaISelLowering.cpp:181
+      // physical registers into virtual ones
+      unsigned Reg = MF.addLiveIn(VA.getLocReg(), RC);
+      SDValue ArgValue = DAG.getCopyFromReg(Chain, DL, Reg, RegVT);
----------------
use Register


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

https://reviews.llvm.org/D145658