[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/lib/Target/PowerPC/PPCAsmPrinter.cpp:1769
+  CommentString.clear();                                                       \
+  OutStreamer->emitIntValueInHexWithPadding(X, S)
+
----------------
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. 


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1769
+  CommentString.clear();                                                       \
+  OutStreamer->emitIntValueInHexWithPadding(X, S)
+
----------------
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?


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1820
+
+#define PRINTBOOL(Prefix, V, Field)                                            \
+  CommentOS << Prefix << (V & (TracebackTable::Field##Mask) ? "+" : "-")       \
----------------
jasonliu wrote:
> Undef these macros when you finished use with it.
I would suggest to rename this macro. As we don't do the "printing" here. I get confused of this when I read we have another OUTPUTCOMMENT later. 
This is more like SAVEBOOLCOMMENT.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1898
+  SecondHalfOfMandatoryField |= (FPRSaved << TracebackTable::FPRSavedShift) &
+                                TracebackTable::FPRSavedMask;
+
----------------
Not sure why we still modify the FPRSaved area in `SecondHalfOfMandatoryField` when we already "print out" FPRSaved above?


================
Comment at: llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.h:26
+public:
+  enum ParaType { FixedType = 0, ShortFloatPoint = 2, LongFloatPoint = 3 };
+
----------------
nit: Para -> Param
I think Param is the most used short form and we should use that.


================
Comment at: llvm/test/CodeGen/PowerPC/aix-emit-tracebacktable.ll:118
+; COMMON-NEXT:                                        #  OnConditionDirective = 0,-IsCRSaved,-IsLRSaved
+; COMMON-NEXT:  .byte   0x80                            #  +IsBackChainStored,-IsFixup,NumOfFPRsSaved = 0
+; COMMON-NEXT:  .byte   0x00                            #  -HasExtensionTable,-HasVectorInfo,NumOfGPRsSaved = 0
----------------
The algorithm of getting NumOfFPRsSaved and NumOfGPRsSaved are not actually put to the test.


================
Comment at: llvm/test/CodeGen/PowerPC/aix-emit-tracebacktable.ll:162
+
+;  CHECKDIS:            ac: 00 00 00 00                  # Traceback table begin
+; CHECKDIS-NEXT:       b0: 00                           # Version = 0
----------------
I would prefer to leave the object file generation and disassemble of it out of this patch. Otherwise, we would have to land the disassemble patch first before we could land this one. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92398