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

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: llvm/include/llvm/BinaryFormat/XCOFF.h:407
 
+StringRef getTracebackTableLanguage(TracebackTable::LanguageID LangId);
+
----------------
`getNameForTracebackTableLanguageId` or maybe just `getDisplayName`. In any case, it's a name that's retrieved and not a "language".


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1775
+  TracebackTable::LanguageID LanguageIdentifier =
+      TracebackTable::CPlusPlus; // C++
+  CommentOS << "Language = " << getTracebackTableLanguage(LanguageIdentifier);
----------------
There should be a comment as to why this is considered the conservatively correct setting (between C and C++) when there is a lack of information in the IR to assist with determining the source language.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1781
+
+  // Emit the 3th byte of the mandatory field.
+
----------------
s/3th/3rd/;


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1790
+  // Check the function uses floating-point processor instructions or not
+  for (unsigned Reg = PPC::F0; Reg <= PPC::F31; Reg++) {
+    if (MRI.isPhysRegUsed(Reg)) {
----------------
Use prefix `++`.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1790
+  // Check the function uses floating-point processor instructions or not
+  for (unsigned Reg = PPC::F0; Reg <= PPC::F31; Reg++) {
+    if (MRI.isPhysRegUsed(Reg)) {
----------------
hubert.reinterpretcast wrote:
> Use prefix `++`.
There are signs that XL will consider usage of the VSX registers that overlay the floating point registers as FP_PRESENT.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1798
+#define GENBOOLCOMMENT(Prefix, V, Field)                                       \
+  CommentOS << Prefix << (V & (TracebackTable::Field##Mask) ? "+" : "-")       \
+            << #Field
----------------
Add parens around `Prefix` and `V`.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1801
+
+#define GENVALUECOMMENT(PrefixiAndName, V, Field)                              \
+  CommentOS << PrefixiAndName << " = "                                         \
----------------
What is "Prefixi" supposed to mean?


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1802-1803
+#define GENVALUECOMMENT(PrefixiAndName, V, Field)                              \
+  CommentOS << PrefixiAndName << " = "                                         \
+            << static_cast<unsigned>((V & (TracebackTable::Field##Mask)) >>    \
+                                     (TracebackTable::Field##Shift))
----------------
Same comment about parens.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1806
+
+  GENBOOLCOMMENT("", FirstHalfOfMandatoryField, IsGlobaLinkage);
+  GENBOOLCOMMENT(", ", FirstHalfOfMandatoryField, IsOutOfLineEpilogOrPrologue);
----------------
Globa(no l)Linkage?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92398