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

Mon Dec 7 15:10:31 PST 2020

jasonliu added inline comments.

================
Comment at: llvm/include/llvm/BinaryFormat/XCOFF.h:21
 namespace llvm {
 class StringRef;

----------------
Could you forward declare the SmallString class and get rid of the header inclusion above?

> template <unsigned InternalLen> class SmallString;

================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:202
+
+  void emitTracebackTable();
 };
----------------
Could this function be private? 

================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1741
+
+  if (!TM.getXCOFFTracebackTable())
+    return;
----------------
nit: a slight preference to have this early return inside of `emitTracebackTable()` function. 

================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1760
+
+  auto outputComment = [&]() {
+    OutStreamer->AddComment(CommentOS.str());
----------------
nit: I would prefer the name to be EmitComment and EmitCommentAndValue.

================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1782
+  if (MF->getFunction().hasExternalLinkage())
+    FirstHalfOfMandatoryField |= TracebackTable::IsGlobaLinkageMask;
+
----------------
I don't think IsGlobalLinkage is mapped to hasExternalLinkage() here.
Global linkage on AIX is referring to the glue code that enables intermodule calls via TOC, which should have mapping class as [GL].
Since we don't generate it, it should be safe for it to be false.

================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1788
+  if (MF->getFunction().hasInternalLinkage())
+    FirstHalfOfMandatoryField |= TracebackTable::IsInternalProcedureMask;
+
----------------
I'm not sure what internal procedure means here. But it seems xlC/xlclang compilers does not set it for static functions. 
So should we error on the safe side and not set it as well?

================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1819
+#define GENVALUECOMMENT(Prefix, V, Field, Name)                                \
+  CommentOS << Prefix << #Name << " = "                                        \
+            << static_cast<unsigned>((V & (TracebackTable::Field##Mask)) >>    \
----------------
You could pass the name in directly as a string without needing to stringfy it in the macro. 
I think that's better because people who looked at the caller could tell it's a string.
For example:
GENVALUECOMMENT(", ", SecondHalfOfMandatoryField, FPRSaved, "NumOfFPRsSaved");

================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1974
+    uint16_t NameLength = Name.size();
+    CommentOS << "Function name len = " << (unsigned int)NameLength;
+    outputCommentAndValue(NameLength, 2);
----------------
use static_cast instead.

================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1981
+  if (FirstHalfOfMandatoryField & TracebackTable::IsAllocaUsedMask) {
+    uint8_t Alloc_Reg =
+        Subtarget->isPPC64() ? PPC::X31 - PPC::X0 : PPC::R31 - PPC::R0;
----------------
nit: Alloc_Reg -> AllocReg.

================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1982
+    uint8_t Alloc_Reg =
+        Subtarget->isPPC64() ? PPC::X31 - PPC::X0 : PPC::R31 - PPC::R0;
+    OutStreamer->AddComment("AllocaUsed");
----------------
This seems to be just fixed value. You don't really need to compute them?

================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1769
+  CommentString.clear();                                                       \
+  OutStreamer->emitIntValueInHexWithPadding(X, S)
+
----------------
DiggerLin wrote:
> DiggerLin wrote:
> > jasonliu wrote:
> > > jasonliu wrote:
> > > > I don't think it's worth to add this function/macro if it just saves one extra line `OutStreamer->emitIntValueInHexWithPadding(X, S)`. And it's actually more helpful for people to see that extra line and know a byte have been printed. 
> > > Do you really need to emit these values with Paddings?
> > I am prefer with paddings.
> it save two lines. and there are several places invoke OutStreamer->emitIntValueInHexWithPadding(X, S), I think it worth it. if I want to change the format of the output, I only need change the macro , I do not several place, for example, if you do not agree with pad, I only need to change the macro.
Any reason you need to emit with Paddings though?
If not, why not just call `emitIntValue` instead.

================
Comment at: llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.h:114

+  /// NumFixedParam - Number of Fixed parameter.
+  unsigned NumFixedParam = 0;
----------------
nit: Fixed -> fixed

================
Comment at: llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.h:117
+
+  /// NumFloatPara - Number of float point parameter.
+  unsigned NumFloatPointParam = 0;
----------------
NumFloatPara -> NumFloatingPointParam
float point -> floating point

================
Comment at: llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.h:120-121
+
+  /// ParamType - Encode type for each parameter pass.
+  /// in ord of parameter pass.
+  /// '0'b => fixed parameter.
----------------

================
Comment at: llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.h:123
+  /// '0'b => fixed parameter.
+  /// '10'b => float point short parameter.
+  /// '11'b => float point long parameter.
----------------
float point -> floating point.

================
Comment at: llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.h:211
+  unsigned getFixedParameter() const { return NumFixedParam; }
+  void incFixedParameter() { NumFixedParam++; }
+
----------------
We should rename  `getFixedParameter` to `getNumFixedParam` so that the function name and member definition matches.

================
Comment at: llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.h:213
+
+  unsigned getFloatPointParameter() const { return NumFloatPointParam; }
+  void incFloatPointParameter() { NumFloatPointParam++; }
----------------
getFloatPointParameter() -> getNumFloatingPointParam()

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92398