This is an archive of the discontinued LLVM Phabricator instance.

[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
jasonliu added inline comments.Dec 2 2020, 1:03 PM
llvm/include/llvm/Target/TargetMachine.h
275–277

I don't think it's necessary for us to mention about asm or XCOFF object file.

llvm/include/llvm/Target/TargetOptions.h
250

No need to mention about asm or object file.

llvm/lib/CodeGen/CommandFlags.cpp
358

Same as above, no need to mention about asm or object file.

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1746

Could we put traceback table emission into its own function(e.g. emitTracebackTable() )? And call that function in emitFunctionBodyEnd() instead?
I think that would make the code cleaner and express the intent better.

1764

Use a lambda instead if you don't need to take the advantage of a macro.

1773

Do you need the cast? If you really do, please use static_cast.

1779

Is there any macro trick we could perform here, instead of having another array of literals?
e.g. could we stringyfy the enum you have in XCOFF.h?

1782–1783

Could you declare the variables near where you would use them?

1818

Undef these macros when you finished use with it.

1824

No c-style cast please.

1986

Why do you need OUTPUTCOMMENT here?

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
3594

Please rebase on the latest master, this part does not apply any more.

llvm/include/llvm/BinaryFormat/XCOFF.h
312–316

LLVM's use of CamelCase includes capitalizing acronyms in the same style as non-acronym words.

317–318

Both PL8 and PLIX use the same value.

llvm/include/llvm/Target/TargetMachine.h
275–277

s/xcoff/XCOFF/; in comments and messages.

llvm/include/llvm/Target/TargetOptions.h
250

Same comment re: "XCOFF".

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1740–1742

Match AIX documentation on style used for language names (for documented cases).

jasonliu added inline comments.Dec 3 2020, 7:14 AM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
3594

Also, it seems test/CodeGen/PowerPC/aix-cc-ext-vec-abi.ll fails with this patch because of the newly added vector register support. Please fix that after rebase.

jasonliu added inline comments.Dec 3 2020, 12:05 PM
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1767

I don't think it's worth to add this function/macro if it just saves one extra line OutStreamer->emitIntValueInHexWithPadding(X, S). And it's actually more helpful for people to see that extra line and know a byte have been printed.

1767

Do you really need to emit these values with Paddings?

1818

I would suggest to rename this macro. As we don't do the "printing" here. I get confused of this when I read we have another OUTPUTCOMMENT later.
This is more like SAVEBOOLCOMMENT.

1896

Not sure why we still modify the FPRSaved area in SecondHalfOfMandatoryField when we already "print out" FPRSaved above?

llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.h
26

nit: Para -> Param
I think Param is the most used short form and we should use that.

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

The algorithm of getting NumOfFPRsSaved and NumOfGPRsSaved are not actually put to the test.

163

I would prefer to leave the object file generation and disassemble of it out of this patch. Otherwise, we would have to land the disassemble patch first before we could land this one.

DiggerLin marked 24 inline comments as done.Dec 4 2020, 7:30 AM
DiggerLin added inline comments.
llvm/include/llvm/BinaryFormat/XCOFF.h
317–318

thanks

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1746

good idea , thanks

1767

I am prefer with paddings.

1767

it save two lines. and there are several places invoke OutStreamer->emitIntValueInHexWithPadding(X, S), I think it worth it. if I want to change the format of the output, I only need change the macro , I do not several place, for example, if you do not agree with pad, I only need to change the macro.

1773

raw_string_ostream do not accept the operator << (uint8_t ) ,

1818

change to GENBOOLCOMMENT and GENVALUECOMMENT

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

I think we need to land the dissemble patch first. for the patch is based on the dissemble patch.

DiggerLin updated this revision to Diff 309549.Dec 4 2020, 8:56 AM
DiggerLin marked 7 inline comments as done.

address comment.

jasonliu added inline comments.Dec 4 2020, 12:45 PM
llvm/include/llvm/Target/TargetOptions.h
133–136

Is this the wrong default? By default llc should emit traceback table for XCOFF object file.

jasonliu added inline comments.Dec 4 2020, 12:56 PM
llvm/test/CodeGen/PowerPC/aix-emit-tracebacktable.ll
108

To match the style with the llvm-objdump, we would want to print a space after ,.

109

Extra space printed after #.

117

Extra # here and the line below.

DiggerLin added inline comments.Dec 4 2020, 2:01 PM
llvm/include/llvm/Target/TargetOptions.h
133–136

if this the default should be emit traceback table. why we need the option -xcoff-traceback-table ? instead we need -disable-xcoff-traceback-table.

jasonliu added inline comments.Dec 4 2020, 2:49 PM
llvm/include/llvm/Target/TargetOptions.h
133–136

It's the same thing. Just different naming and switched the default value.
You could have -disable-xcoff-traceback-table if you want, but it's just longer name.
Also, we would need a driver option later, and I suppose the driver option is something like -fxcoff-traceback-table and -fno-xcoff-traceback-table, with -fxcoff-traceback-table being default to true on AIX. So with the current name, it's going to match the driver option better.

DiggerLin marked 3 inline comments as done.Dec 7 2020, 8:11 AM
DiggerLin updated this revision to Diff 309924.Dec 7 2020, 8:24 AM

address comment

jasonliu added inline comments.Dec 7 2020, 3:10 PM
llvm/include/llvm/BinaryFormat/XCOFF.h
20–21

Could you forward declare the SmallString class and get rid of the header inclusion above?

template <unsigned InternalLen> class SmallString;

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
204

Could this function be private?

1741

nit: a slight preference to have this early return inside of emitTracebackTable() function.

1760

nit: I would prefer the name to be EmitComment and EmitCommentAndValue.

1767

Any reason you need to emit with Paddings though?
If not, why not just call emitIntValue instead.

1782

I don't think IsGlobalLinkage is mapped to hasExternalLinkage() here.
Global linkage on AIX is referring to the glue code that enables intermodule calls via TOC, which should have mapping class as [GL].
Since we don't generate it, it should be safe for it to be false.

1788

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?

1819

You could pass the name in directly as a string without needing to stringfy it in the macro.
I think that's better because people who looked at the caller could tell it's a string.
For example:
GENVALUECOMMENT(", ", SecondHalfOfMandatoryField, FPRSaved, "NumOfFPRsSaved");

1974

use static_cast instead.

1981

nit: Alloc_Reg -> AllocReg.

1982

This seems to be just fixed value. You don't really need to compute them?

llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.h
114

nit: Fixed -> fixed

117

NumFloatPara -> NumFloatingPointParam
float point -> floating point

120–121
123

float point -> floating point.

211

We should rename getFixedParameter to getNumFixedParam so that the function name and member definition matches.

213

getFloatPointParameter() -> getNumFloatingPointParam()

DiggerLin marked 13 inline comments as done.Dec 8 2020, 10:11 AM
DiggerLin added inline comments.
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1741

putting if (!TM.getXCOFFTracebackTable()) here, it do not need extra function call emitTracebackTable when getXCOFFTracebackTable is false.

1767

for example , if the value is 3, if use the emitIntValue. it will be display
.byte 0x3,
with
emitIntValueInHexWithPadding will show as 0x03. I can know , it is only byte value.

if it is two bytes value , it will show as 0x0003. even there is (.vbytes 2, 0x3), but I prefer show as (.vbytes 2, 0x0003) .

DiggerLin updated this revision to Diff 310282.Dec 8 2020, 10:46 AM
DiggerLin marked 2 inline comments as done.

address comment

jasonliu added inline comments.Dec 8 2020, 2:24 PM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
7310

Do you actually need to traverse to find out exactly what register is used?
Could you use queries in VA.getValVT() to find out whether it's a fixed param or float param?
For example, VA.getValVT().isScalarInteger().

jasonliu added inline comments.Dec 8 2020, 2:35 PM
llvm/lib/BinaryFormat/XCOFF.cpp
101

nit :I think it's fine to return "CPlusPlus" in the string and no need for a special case here.

DiggerLin marked an inline comment as done.Dec 8 2020, 3:19 PM
DiggerLin updated this revision to Diff 310369.Dec 8 2020, 3:23 PM
llvm/lib/BinaryFormat/XCOFF.cpp
113

Minor nit: avoid implicit conversion to bool when comparing against zero for integers.

136

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.

138

Why is this hardcoded (as opposed to a named constant representing the width of the ParmTypeMask)?

llvm/lib/BinaryFormat/XCOFF.cpp
139–141

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.

165

Would a Value == 0u check here make sense? The other function has a check that serves a similar purpose.

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1758

I don't think a std::string in needed here; using raw_svector_ostream should work.

llvm/include/llvm/BinaryFormat/XCOFF.h
407

getNameForTracebackTableLanguageId or maybe just getDisplayName. In any case, it's a name that's retrieved and not a "language".

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1775

There should be a comment as to why this is considered the conservatively correct setting (between C and C++) when there is a lack of information in the IR to assist with determining the source language.

1781

s/3th/3rd/;

1790

Use prefix ++.

1790

There are signs that XL will consider usage of the VSX registers that overlay the floating point registers as FP_PRESENT.

1798

Add parens around Prefix and V.

1801

What is "Prefixi" supposed to mean?

1802–1803

Same comment about parens.

1806

Globa(no l)Linkage?

DiggerLin marked an inline comment as done.Dec 9 2020, 6:26 AM
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1779

Please add at least a comment that this is only populated for the third and fourth bytes.

1788

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.

DiggerLin marked 18 inline comments as done.Dec 9 2020, 1:18 PM
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.
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.

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.

121
125

This corresponds to a field with a specific width.

216

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
67

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

68
70

Prefix ++ please.

71

This is ParmTypeIsFloatingBit?

79

Same comment re: ++.

83–86

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
67

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
67

change to ParamNo

67

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
70

@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
7310–7314

Minor nit: braces no longer needed.

llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.cpp
81

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

95

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
95

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
96

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

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
97