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

[Digger via Phabricator via llvm-commits]

DiggerLin marked 3 inline comments as done.
DiggerLin 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!");
----------------
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.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92398