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

Thu Dec 10 08:44:26 PST 2020

jasonliu added inline comments.

================
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!");
----------------
DiggerLin wrote:
> hubert.reinterpretcast wrote:
> > DiggerLin wrote:
> > > 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.
> > > yes, the ParmsNum in function  only includes the fixed  and floating parameter count currently. 
> > This is an interface surprise that should have copious comments if it were to be left around.
> > 
> > > 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 .
> > If this is true, then it's worth having a comment where the `++I` is intentionally omitted.
> > 
> > > actually if we only put fixed parameter count as parameter of the function, we can also decode the ParmsType correctly by Value != 0u.
> > There are 8 GPRs and 12 VRs for function arguments. So, just with registers, there can be more than the 16 parameters that can be encoded in the 32 bits. The assert can be hit and it's not always an error. Trying to say that there is an infinite number of `0` bits to the right may be too clever because it makes it difficult to draw the line for other unambiguous situations involving having only vector parameters left to account for. An alternative solution where the number of encoding bits left is tracked and unencoded parameters are represented by an ellipsis would be something to consider.
> > 
> > > 
> > > 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.
> > Thanks for your enlightening response. I think we have more to fix here.
> > > yes, the ParmsNum in function  only includes the fixed  and floating parameter count currently. 
> > This is an interface surprise that should have copious comments if it were to be left around.
> > 
> > > 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 .
> > If this is true, then it's worth having a comment where the `++I` is intentionally omitted.
> > 
> > > actually if we only put fixed parameter count as parameter of the function, we can also decode the ParmsType correctly by Value != 0u.
> > There are 8 GPRs and 12 VRs for function arguments. So, just with registers, there can be more than the 16 parameters that can be encoded in the 32 bits. The assert can be hit and it's not always an error. Trying to say that there is an infinite number of `0` bits to the right may be too clever because it makes it difficult to draw the line for other unambiguous situations involving having only vector parameters left to account for. An alternative solution where the number of encoding bits left is tracked and unencoded parameters are represented by an ellipsis would be something to consider.
> >>> Actually we do not use the function in the patch, we just move it the function from xcoffobject.cpp file to here, We can change it in a NFC patch or in encoding vector info patch later. 
> > 
> > > 
> > > 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.
> > Thanks for your enlightening response. I think we have more to fix here.
> 
> 
@DiggerLin I don't really see this part of the comment being addressed:
```
There are 8 GPRs and 12 VRs for function arguments. So, just with registers, there can be more than the 16 parameters that can be encoded in the 32 bits. The assert can be hit and it's not always an error. Trying to say that there is an infinite number of 0 bits to the right may be too clever because it makes it difficult to draw the line for other unambiguous situations involving having only vector parameters left to account for. An alternative solution where the number of encoding bits left is tracked and unencoded parameters are represented by an ellipsis would be something to consider.
```
But I don't think this function actually gets called in this patch. So maybe we should consider to remove this function out of this patch.

================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1788
+  if (MF->getFunction().hasInternalLinkage())
+    FirstHalfOfMandatoryField |= TracebackTable::IsInternalProcedureMask;
+
----------------
hubert.reinterpretcast wrote:
> jasonliu wrote:
> > 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?
> Just for information: an "internal procedure" in this context is one that does not manipulate the stack pointer. Procedures that fit this description nevertheless don't have the flag set by `xlc`, etc.
Okay; Thanks. So it's like a leaf function. 

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92398