Page MenuHomePhabricator

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

Authored by DiggerLin on Dec 1 2020, 8:47 AM.

Details

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.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
DiggerLin added inline comments.Dec 9 2020, 1:18 PM
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 marked 7 inline comments as done.Dec 9 2020, 1:45 PM
DiggerLin added inline comments.
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1806

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

DiggerLin updated this revision to Diff 310648.Dec 9 2020, 1:50 PM

address comment

DiggerLin marked an inline comment as done.Dec 9 2020, 6:52 PM
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.

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 updated this revision to Diff 310881.Dec 10 2020, 7:05 AM
DiggerLin marked 5 inline comments as done.

address comment

DiggerLin marked 3 inline comments as done.Dec 10 2020, 8:19 AM
DiggerLin added inline comments.
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 added inline comments.Dec 10 2020, 8:44 AM
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.

DiggerLin marked 3 inline comments as done.
jasonliu accepted this revision.Dec 10 2020, 10:27 AM

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

This revision is now accepted and ready to land.Dec 10 2020, 10:27 AM
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 updated this revision to Diff 311041.Dec 10 2020, 3:40 PM
DiggerLin marked 5 inline comments as done.
DiggerLin added inline comments.
llvm/lib/BinaryFormat/XCOFF.cpp
107

ok, thanks

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.

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.

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.

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 marked 15 inline comments as done.Dec 11 2020, 8:54 AM
DiggerLin added inline comments.
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 updated this revision to Diff 311241.Dec 11 2020, 9:01 AM

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

DiggerLin marked an inline comment as done.Dec 11 2020, 9:19 AM
DiggerLin updated this revision to Diff 311252.Dec 11 2020, 9:22 AM

change function from setParameterType to appendParameterType

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.

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.

DiggerLin marked 4 inline comments as done.Dec 11 2020, 10:19 AM
DiggerLin marked an inline comment as done.Dec 11 2020, 10:37 AM

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 marked 3 inline comments as done.Dec 11 2020, 1:00 PM
DiggerLin added inline comments.
llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.cpp
96

yes, you are correct. thanks

This revision was landed with ongoing or failed builds.Dec 11 2020, 2:50 PM
This revision was automatically updated to reflect the committed changes.
DiggerLin marked an inline comment as done.
RKSimon added inline comments.
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 added inline comments.
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 marked 2 inline comments as done.Dec 14 2020, 8:45 AM
DiggerLin added inline comments.
llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.cpp
98