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

[Digger via Phabricator via llvm-commits]

DiggerLin marked 24 inline comments as done.
DiggerLin added inline comments.


================
Comment at: llvm/include/llvm/BinaryFormat/XCOFF.h:311-312
+    RPG,
+    PL8,
+    PLIX,
+    Assembly,
----------------
hubert.reinterpretcast wrote:
> Both PL8 and PLIX use the same value.
thanks


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1748
+
+  if (!TM.getXCOFFTracebackTable())
+    return;
----------------
jasonliu wrote:
> 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. 
good idea , thanks


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


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


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


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1820
+
+#define PRINTBOOL(Prefix, V, Field)                                            \
+  CommentOS << Prefix << (V & (TracebackTable::Field##Mask) ? "+" : "-")       \
----------------
jasonliu wrote:
> 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.
change to GENBOOLCOMMENT and GENVALUECOMMENT


================
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
----------------
jasonliu wrote:
> 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. 
I think we need to land the dissemble patch first. for the patch is based on the dissemble patch. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92398