- added a new option -xcoff-traceback-table to control whether generate traceback table for function.
- implement the functionality of emit traceback table of a function.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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? | |
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? | |
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/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. |
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. | |
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 | |
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. |
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. |
llvm/include/llvm/Target/TargetOptions.h | ||
---|---|---|
133–136 | Is this the wrong default? By default llc should emit traceback table for XCOFF object file. |
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. |
llvm/include/llvm/Target/TargetOptions.h | ||
---|---|---|
133–136 | It's the same thing. Just different naming and switched the default value. |
llvm/include/llvm/BinaryFormat/XCOFF.h | ||
---|---|---|
20–21 | Could you forward declare the SmallString class and get rid of the header inclusion above?
| |
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? | |
1782 | I don't think IsGlobalLinkage is mapped to hasExternalLinkage() here. | |
1788 | I'm not sure what internal procedure means here. But it seems xlC/xlclang compilers does not set it for static functions. | |
1819 | You could pass the name in directly as a string without needing to stringfy it in the macro. | |
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 | |
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() |
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 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) . |
llvm/lib/Target/PowerPC/PPCISelLowering.cpp | ||
---|---|---|
7310 | Do you actually need to traverse to find out exactly what register is used? |
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. |
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? |
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. |
llvm/lib/BinaryFormat/XCOFF.cpp | ||
---|---|---|
139–141 | yes, the ParmsNum in function only includes the fixed and floating parameter count currently. 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. |
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. 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. 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; } | |
1801 | change to prefix | |
1806 | David Edelsohn told me in the gcc |
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp | ||
---|---|---|
1806 | yes , it is typo error, I will create a NFC later to deal with it later. |
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 |
This is an interface surprise that should have copious comments if it were to be left around.
If this is true, then it's worth having a comment where the ++I is intentionally omitted.
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.
Thanks for your enlightening response. I think we have more to fix here. |
llvm/lib/BinaryFormat/XCOFF.cpp | ||
---|---|---|
139–141 |
|
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. |
Thanks. LGTM.
Please allow some time for @hubert.reinterpretcast to go over again before you land.
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. |
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. |
address comment including fixing a paramtype bug and adding a new content in test case.
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. |
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. |
llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.cpp | ||
---|---|---|
95 | yes, you are correct. thanks |
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 ? |
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 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' |
llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.cpp | ||
---|---|---|
97 |
Could you forward declare the SmallString class and get rid of the header inclusion above?