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

[Digger via Phabricator via llvm-commits]

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


================
Comment at: llvm/lib/BinaryFormat/XCOFF.cpp:136
+    default:
+      assert(false && "Unrecognized bits in ParmsType.");
+    }
----------------
hubert.reinterpretcast wrote:
> Is `assert` right for all expected callers of this function? This seems to be a reusable implementation where the error handling policy should be left to the caller.
the code should not came to default.  added assert here we don't get surprised.


================
Comment at: llvm/lib/BinaryFormat/XCOFF.cpp:139-141
+  assert(I == ParmsNum &&
+         "The total parameters number of fixed-point or floating-point "
+         "parameters not equal to the number in the parameter type!");
----------------
hubert.reinterpretcast wrote:
> The message text here is wrong. The correct `ParmsNum` here needs to include the number of vector parameters, which the XL compilers consider as neither "fixed-point" nor floating-point.
> 
> @DiggerLin, please confirm that the caller of this function correctly includes the vector parameter count.
> 
> Also, same comment as above re: `assert`.
yes, the ParmsNum in function  only includes the fixed  and floating parameter count currently. 
so in the function , when decoding we  do not have  ++I in the 
"case TracebackTable::ParmTypeIsVectorBits" , we  grantee all the vector parameter meter will be decoded by the  Value != 0u . actually if we only put fixed parameter count as parameter of the function, we can also decode the ParmsType correctly by Value != 0u.

that is why the  message text is "The total parameters number of fixed-point or floating-point " not have vector ,  

if you not agreed with only pass  fixed  and floating parameter  count, I can create a NFC patch later.


================
Comment at: llvm/lib/BinaryFormat/XCOFF.cpp:165
+  }
+  return ParmsType;
+}
----------------
hubert.reinterpretcast wrote:
> Would a `Value == 0u` check here make sense? The other function has a check that serves a similar purpose.
Added, thanks.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1758
+  std::string CommentString;
+  raw_string_ostream CommentOS(CommentString);
+
----------------
hubert.reinterpretcast wrote:
> I don't think a `std::string` in needed here; using `raw_svector_ostream` should work.
using raw_svector_ostream here. it need SmallString ,  for example.
SmallString<128> InstPrinterStr; 
    raw_svector_ostream OSS(InstPrinterStr);

if you think we need to raw_svector_ostream  , I can move to raw_svector_ostream.  


================
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:
> 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.
Yes, I think isPhysRegUsed(Reg) will also let usage of the VSX registers that overlay the floating point registers as FP_PRESENT too.
in the function 
bool MachineRegisterInfo::isPhysRegUsed(MCRegister PhysReg) const {
  if (UsedPhysRegMask.test(PhysReg))
    return true;
  const TargetRegisterInfo *TRI = getTargetRegisterInfo();
  for (MCRegAliasIterator AliasReg(PhysReg, TRI, true); AliasReg.isValid();
       ++AliasReg) {
    if (!reg_nodbg_empty(*AliasReg))
      return true;
  }
  return false;
}
it will iterator all the overlay registers.



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


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1806
+
+  GENBOOLCOMMENT("", FirstHalfOfMandatoryField, IsGlobaLinkage);
+  GENBOOLCOMMENT(", ", FirstHalfOfMandatoryField, IsOutOfLineEpilogOrPrologue);
----------------
hubert.reinterpretcast wrote:
> Globa(no l)Linkage?
David Edelsohn told me in the gcc 
The “global linkage”, “out-of-line prologue/epilogue”, “internal function”, “controlled storage”, and “function has no toc” are unset.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92398