[PATCH] D145660: [Xtensa] Codegen support for memory operations

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added a comment.

Basically lgtm with some nits



================
Comment at: llvm/lib/Target/Xtensa/XtensaISelDAGToDAG.cpp:55
+    if (TM.isPositionIndependent())
+      report_fatal_error("PIC relocations is not supported");
+
----------------
s/is not/are not/

Can you make this a DiagnosticInfoUnsupported instead?


================
Comment at: llvm/lib/Target/Xtensa/XtensaISelDAGToDAG.cpp:67-81
+      switch (Scale) {
+      case 1:
+        Valid = (OffsetVal >= 0 && OffsetVal <= 255);
+        break;
+      case 2:
+        Valid =
+            (OffsetVal >= 0 && OffsetVal <= 510) && ((OffsetVal & 0x1) == 0);
----------------
Move this to a separate util function


================
Comment at: llvm/lib/Target/Xtensa/XtensaRegisterInfo.cpp:67-68
+  MachineInstr &MI = *II;
+  unsigned FrameReg;
+  FrameReg = Xtensa::SP;
+
----------------
define and init on same line?


================
Comment at: llvm/lib/Target/Xtensa/XtensaRegisterInfo.cpp:80
+
+  Offset = SPOffset + (int64_t)StackSize;
+  Offset += MI.getOperand(OpNo + 1).getImm();
----------------
init on definition


================
Comment at: llvm/lib/Target/Xtensa/XtensaRegisterInfo.cpp:86-100
+  bool Valid = false;
+  switch (MI.getOpcode()) {
+  case Xtensa::L8UI:
+  case Xtensa::S8I:
+    Valid = (Offset >= 0 && Offset <= 255);
+    break;
+  case Xtensa::L16SI:
----------------
Move to separate util function


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

https://reviews.llvm.org/D145660