[PATCH] D64829: [Xtensa 3/10] Add initial version of the Xtensa backend.

[Sergei Barannikov via Phabricator via llvm-commits]

barannikov88 added inline comments.


================
Comment at: llvm/lib/Target/Xtensa/MCTargetDesc/XtensaMCTargetDesc.cpp:13
+// We need to define this function for linking succeed
+extern "C" void LLVMInitializeXtensaTargetMC() {}
----------------
Global initialization function should use LLVM_EXTERNAL_VISIBILITY.


================
Comment at: llvm/lib/Target/Xtensa/TargetInfo/CMakeLists.txt:1
+include_directories( ${CMAKE_CURRENT_BINARY_DIR}/.. ${CMAKE_CURRENT_SOURCE_DIR}/.. )
+
----------------
This shouldn't be necessary, these directory are already available for include directives.


================
Comment at: llvm/lib/Target/Xtensa/TargetInfo/XtensaTargetInfo.cpp:17
+}
+extern "C" void LLVMInitializeXtensaTargetInfo() {
+  RegisterTarget<Triple::xtensa> X(TheXtensaTarget, "xtensa", "Xtensa 32",
----------------
LLVM_EXTERNAL_VISIBILITY


================
Comment at: llvm/lib/Target/Xtensa/XtensaTargetMachine.cpp:27
+
+extern "C" void LLVMInitializeXtensaTarget() {
+  // Register the target.
----------------
LLVM_EXTERNAL_VISIBILITY


================
Comment at: llvm/lib/Target/Xtensa/XtensaTargetMachine.h:23
+
+class TargetFrameLowering;
+
----------------
Appears unused.


================
Comment at: llvm/lib/Target/Xtensa/XtensaTargetMachine.h:25
+
+extern Target TheXtensaTarget;
+
----------------
MaskRay wrote:
> Other targets use a static local variable in `llvm::get*Target` but this seems fine.
(Optional) This is gonna be used in multiple files, it might worth moving it ino a header file.



================
Comment at: llvm/lib/Target/Xtensa/XtensaTargetMachine.h:28
+class XtensaTargetMachine : public LLVMTargetMachine {
+  std::unique_ptr<TargetLoweringObjectFile> TLOF;
+
----------------
MaskRay wrote:
> include llvm/Target/TargetLoweringObjectFile.h for this
This should not be necessary as long as the destructor of XtensaTargetMachine is in *.cpp file. Forward declaration should be enough (and it is already forward declared in TargetMachine.h).


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

https://reviews.llvm.org/D64829