- 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/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. | |
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. |
address comment including fixing a paramtype bug and adding a new content in test case.
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. |
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. |
llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.cpp | ||
---|---|---|
96 | yes, you are correct. thanks |
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 ? |
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 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 | ||
---|---|---|
98 |