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

[Andrei Safronov via Phabricator via llvm-commits]

andreisfr 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() {}
----------------
barannikov88 wrote:
> Global initialization function should use LLVM_EXTERNAL_VISIBILITY.
fixed


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


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


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


================
Comment at: llvm/lib/Target/Xtensa/XtensaTargetMachine.cpp:36
+  std::string Ret = "e-m:e-p:32:32-i8:8:32-i16:16:32-i64:64-n32";
+
+  return Ret;
----------------
MaskRay wrote:
> delete blank line. Just return the string.
fixed


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


================
Comment at: llvm/lib/Target/Xtensa/XtensaTargetMachine.h:25
+
+extern Target TheXtensaTarget;
+
----------------
barannikov88 wrote:
> 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.
> 
I added "llvm/lib/Target/Xtensa/TargetInfo/XtensaTargetInfo.h" file with "&getTheXtensaTarget();" function, this approach is similar to other backends.


================
Comment at: llvm/lib/Target/Xtensa/XtensaTargetMachine.h:43
+
+  // Override LLVMTargetMachine
+  TargetPassConfig *createPassConfig(PassManagerBase &PM) override;
----------------
MaskRay wrote:
> delete the comment. `override` self explains.
fixed


================
Comment at: llvm/lib/Target/Xtensa/XtensaTargetMachine.h:51
+
+#endif /* LLVM_LIB_TARGET_XTENSA_XTENSATARGETMACHINE_H */
----------------
MaskRay wrote:
> I think all patches use `/*` footer for header guards, which should all be fixed.
fixed


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

https://reviews.llvm.org/D64829