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

[Sergei Barannikov via Phabricator via llvm-commits]

barannikov88 added a comment.

Half of the includes and forward declarations are unused.
I'm not familiar with the ABI to review this part.
However, manual handling of i1, i8, i16 and f32 looks suspicious, because they are all illegal.
Looks like a dead code to me, but I may be wrong.



================
Comment at: llvm/lib/Target/Xtensa/Xtensa.h:30
+} // namespace llvm
+#endif /* LLVM_LIB_TARGET_XTENSA_XTENSA_H */
----------------
The comment style is always `//`.



================
Comment at: llvm/lib/Target/Xtensa/XtensaAsmPrinter.h:37
+
+  // Override AsmPrinter.
+  StringRef getPassName() const override { return "Xtensa Assembly Printer"; }
----------------
Such comments (in other files too) are obvious from the class declaration.


================
Comment at: llvm/lib/Target/Xtensa/XtensaCallingConv.td:14
+/// CCIfAlign - Match of the original alignment of the arg
+class CCIfAlign<string Align, CCAction A>
+  : CCIf<!strconcat("ArgFlags.getNonZeroOrigAlign() == ", Align), A>;
----------------
Unused.


================
Comment at: llvm/lib/Target/Xtensa/XtensaISelDAGToDAG.cpp:77
+  // Dump information about the Node being selected
+  LLVM_DEBUG(errs() << "Selecting: "; Node->dump(CurDAG); errs() << "\n");
+
----------------
This is already printed by `-debug-only=isel`.


================
Comment at: llvm/lib/Target/Xtensa/XtensaISelDAGToDAG.cpp:81
+  if (Node->isMachineOpcode()) {
+    LLVM_DEBUG(errs() << "== "; Node->dump(CurDAG); errs() << "\n");
+    return;
----------------
Should be `dbgs()`, `errs()` is for errors.


================
Comment at: llvm/lib/Target/Xtensa/XtensaISelDAGToDAG.cpp:82
+    LLVM_DEBUG(errs() << "== "; Node->dump(CurDAG); errs() << "\n");
+    return;
+  }
----------------
`Node->setNodeId(-1)` is missing. It is a subtle bug that is difficult to discover.


================
Comment at: llvm/lib/Target/Xtensa/XtensaISelLowering.cpp:112
+    State.addLoc(CCValAssign::getMem(ValNo, ValVT, Offset, LocVT, LocInfo));
+  } else
+    State.addLoc(CCValAssign::getReg(ValNo, ValVT, Reg, LocVT, LocInfo));
----------------
Missing braces. Around llvm_unreachable above, too.


================
Comment at: llvm/lib/Target/Xtensa/XtensaISelLowering.h:28
+  // Return with a flag operand.  Operand 0 is the chain operand.
+  RET_FLAG
+};
----------------
The `FLAG` notion is historical (Glue was named Flag one day). Please don't copy it into new targets.
This should be just RET or RETURN.



================
Comment at: llvm/lib/Target/Xtensa/XtensaTargetMachine.h:40
 
+  const XtensaSubtarget *getSubtargetImpl() const { return &Subtarget; }
+  const XtensaSubtarget *getSubtargetImpl(const Function &F) const override;
----------------
This method should not exist. Subtarget is per-function.


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

https://reviews.llvm.org/D145658