[PATCH] D92398: [AIX][XCOFF] emit traceback table for function in aix

[Jason Liu via Phabricator via llvm-commits]

jasonliu added inline comments.


================
Comment at: llvm/include/llvm/Target/TargetMachine.h:275-277
+  /// Return true if should emit traceback table for asm in AIX OS or xcoff
+  /// object file
+  /// corresponding to -xcoff-traceback-table.
----------------
I don't think it's necessary for us to mention about asm or XCOFF object file. 


================
Comment at: llvm/include/llvm/Target/TargetOptions.h:250
 
+    /// Emit traceback table for asm in AIX OS or xcoff object file.
+    unsigned XCOFFTracebackTable : 1;
----------------
No need to mention about asm or object file. 


================
Comment at: llvm/lib/CodeGen/CommandFlags.cpp:357
+      cl::desc(
+          "Emit the traceback table for asm in AIX OS or in XCOFF object file"),
+      cl::init(false));
----------------
Same as above, no need to mention about asm or object file. 


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1748
+
+  if (!TM.getXCOFFTracebackTable())
+    return;
----------------
Could we put traceback table emission into its own function(e.g. `emitTracebackTable()` )? And call that function in emitFunctionBodyEnd() instead? 
I think that would make the code cleaner and express the intent better. 


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1766
+
+#define OUTPUTCOMMENTVALUE(X, S)                                               \
+  OutStreamer->AddComment(CommentOS.str());                                    \
----------------
Use a lambda instead if you don't need to take the advantage of a macro.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1775
+
+  CommentOS << "Version = " << (unsigned int)Version;
+  OUTPUTCOMMENTVALUE(Version, 1);
----------------
Do you need the cast? If you really do, please use static_cast.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1781
+         "Unknown Language ID.");
+  CommentOS << "Language = " << SourceLanguageIdentifier[LanguageIdentifier];
+  OUTPUTCOMMENTVALUE(LanguageIdentifier, 1);
----------------
Is there any macro trick we could perform here, instead of having another array of literals?
e.g. could we stringyfy the enum you have in XCOFF.h?


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1784-1785
+
+  const PPCFunctionInfo *FI = MF->getInfo<PPCFunctionInfo>();
+  const MachineRegisterInfo &MRI = MF->getRegInfo();
+
----------------
Could you declare the variables near where you would use them?


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1820
+
+#define PRINTBOOL(Prefix, V, Field)                                            \
+  CommentOS << Prefix << (V & (TracebackTable::Field##Mask) ? "+" : "-")       \
----------------
Undef these macros when you finished use with it.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1826
+  CommentOS << Prefix << #Name << " = "                                        \
+            << (unsigned)(V & (TracebackTable::Field##Mask) >>                 \
+                                  (TracebackTable::Field##Shift))
----------------
No c-style cast please.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1988
+    OutStreamer->AddComment("AllocaUsed");
+    OUTPUTCOMMENT;
+    OutStreamer->emitIntValueInHex(Alloc_Reg, sizeof(Alloc_Reg));
----------------
Why do you need `OUTPUTCOMMENT` here?


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:3578
+static const MCPhysReg GPR_64[] = { // 64-bit registers.
+    PPC::X3, PPC::X4, PPC::X5, PPC::X6, PPC::X7, PPC::X8, PPC::X9, PPC::X10};
+
----------------
Please rebase on the latest master, this part does not apply any more. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92398