[AIX][XCOFF] emit traceback table for function in aix

[DiggerLin]

Summary

1. added a new option -xcoff-traceback-table to control whether generate traceback table for function.
2. implement the functionality of emit traceback table of a function.

[DiggerLin]

llvm/lib/BinaryFormat/XCOFF.cpp

136

the code should not came to default. added assert here we don't get surprised.

165

Added, thanks.

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

1758

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.

1790

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.

1801

change to prefix

1806

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.

[DiggerLin]

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

1806

yes , it is typo error, I will create a NFC later to deal with it later.

[DiggerLin]

address comment

[hubert.reinterpretcast]

llvm/lib/BinaryFormat/XCOFF.cpp

136

Okay; this is not just "should". It's actually impossible unless if the constants are incorrectly defined. Got it; thanks.

139–141

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.

[hubert.reinterpretcast]

llvm/include/llvm/BinaryFormat/XCOFF.h

33

Seems weird to me that this is not defined as part of TracebackTable.

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

1758

Yes, please make that change.

1775

We use C++ as the Language here because ...? Please ask @jasonliu if help is needed on the explanation.

[DiggerLin]

address comment

[DiggerLin]

llvm/lib/BinaryFormat/XCOFF.cpp

139–141

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.

[jasonliu]

llvm/lib/BinaryFormat/XCOFF.cpp

139–141

@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.

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

1788

Okay; Thanks. So it's like a leaf function.

[jasonliu]

Thanks. LGTM.
Please allow some time for @hubert.reinterpretcast to go over again before you land.

[hubert.reinterpretcast]

llvm/lib/BinaryFormat/XCOFF.cpp

107

When we revisit for vector parameters, we should also revisit this function.

127–128

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

1775

Minor nit: Use period instead of comma.

1867

Minor nit: preincrement.

1889

Minor nit: same comment re: ++.

1899–1900

The values of the constants reflect the XL order but the output order doesn't.

[DiggerLin]

llvm/lib/BinaryFormat/XCOFF.cpp

107

ok, thanks

[hubert.reinterpretcast]

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

1838

Use static_assert to relate the registers being checked against the register we will claim is being associated with alloca.

1939

Assert here that HasVectorInfo is false.

1954–1957

Fix misplaced comment.

1959

Fix typo.

1963

I did not check the XL behaviour here, but it seems that the safe thing to do is to truncate the name to INT16_MAX characters. Also, using int16_t will better match the type of the field.

llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.h

26

There seems to be no requirement for the enumerators to take on these particular values.

127

131

This corresponds to a field with a specific width.

223

Same comment.

[hubert.reinterpretcast]

llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.h

26

Okay, this is used by setParameterType. It seems that the values should be written as hex here alongside comments indicating the bitstrings used in the encoding format.

[hubert.reinterpretcast]

llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.cpp

68

Is RegNo a correct name for this parameter? It's not used like a register number (e.g., PPC::R3).

69

71

Prefix ++ please.

72

This is ParmTypeIsFloatingBit?

80

Same comment re: ++.

84–87

There can be 8 GPRs of arguments followed by 13 FPRs of arguments. 8 + 13 * 2 = 34. Shifting by a negative number of bits is undefined behaviour.

llvm/test/CodeGen/PowerPC/aix-emit-tracebacktable.ll

17

Please add a test where a struct taking more than one register occurs prior to a float.

[hubert.reinterpretcast]

llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.cpp

68

Perhaps this function should use and also be responsible for adjusting FixedParamNum and FloatingPointParamNum. The function should be renamed to something like appendParameterType.

[DiggerLin]

llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.cpp

68

change to ParamNo

68

change the interface from setParameterType(ParamType Type, unsigned RegNo) to setParameterType(ParamType Type)

[DiggerLin]

address comment including fixing a paramtype bug and adding a new content in test case.

[DiggerLin]

change function from setParameterType to appendParameterType

[hubert.reinterpretcast]

llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.cpp

71

@DiggerLin, can you merge the updates for FixedParamNum and FloatingPointParamNum into this function? You might have missed my comment this morning about making this appendParameterType.

[hubert.reinterpretcast]

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

1941

Minor nit: grammar.

1963

Please add a line before getting NameLength to update the length of the StringRef to truncate to INT16_MAX characters.

[hubert.reinterpretcast]

LGTM with comments; thanks.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

7311–7315

Minor nit: braces no longer needed.

llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.cpp

82

Minor nit: since the wrapping behaviour of unsigned is unwanted here, using a signed type assists in expressing the intent.

96

This seems to be an off-by-one error? It's fine to set the rightmost two bits together.

[DiggerLin]

llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.cpp

96

yes, you are correct. thanks

[RKSimon]

llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.cpp

97

@DiggerLin This is causing "comparison of unsigned expression >= 0 is always true" gcc warnings.

Maybe change to:

if (Bits < 31)

or change Bits to an int ?

[morehouse]

llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.cpp

98

This line is causing UBSan errors: http://lab.llvm.org:8011/#/builders/5/builds/2407/steps/2/logs/stdio
Please fix or revert.

FAIL: LLVM :: CodeGen/PowerPC/aix-cc-abi.ll (45237 of 72201)
******************** TEST 'LLVM :: CodeGen/PowerPC/aix-cc-abi.ll' FAILED ********************
...
Exit Code: 2
Command Output (stderr):
--
/b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.cpp:98:27: runtime error: shift exponent 4294967294 is too large for 32-bit type 'int'

[DiggerLin]

llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.cpp

98

https://reviews.llvm.org/rG15f2d4f198380762e9fcf6b456d405078b87ae7a