[PATCH] D92398: [AIX][XCOFF] emit traceback table for function in aix
Jason Liu via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
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
More information about the llvm-commits
mailing list