Page MenuHomePhabricator

[AIX][XCOFF] generate eh_info when vector registers are saved according to the traceback table.
ClosedPublic

Authored by DiggerLin on Jun 3 2021, 3:07 PM.

Details

Summary

generate eh_info when vector registers are saved according to the traceback table.

struct eh_info_t {

unsigned version;       /* EH info version 0 */

#if defined(64BIT)

char _pad[4];           /* padding */

#endif

unsigned long lsda;     /* Pointer to Language Specific Data Area */
unsigned long personality; /* Pointer to the personality routine */

};

the value of lsda and personality is zero when the number of vector registers saved is large zero and there is not personality of the function

Diff Detail

Event Timeline

DiggerLin created this revision.Jun 3 2021, 3:07 PM
DiggerLin requested review of this revision.Jun 3 2021, 3:07 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 3 2021, 3:07 PM
jasonliu added inline comments.Jun 7 2021, 1:30 PM
llvm/include/llvm/CodeGen/AsmPrinter.h
755 ↗(On Diff #349907)

I don't feel like this function belong in AsmPrinter.
This function feels more like a PPCFunctionInfo class function.
Admittedly, doing that means in AIXException.cpp, you would need to do an include like this: `#include ../../Target/PowerPC/PPCMachineFunctionInfo.h", which I don't see very often in the llvm source.
Not sure if this is an acceptable practice. @hubert.reinterpretcast any ideas?

The other way of doing this without that weird include in AIXException.cpp would be to move the content in AIXException.cpp to PPCAIXAsmPrinter instead. That way, you will definitely have access to everything you need in the PPCAIXAsmPrinter context.

Other than that, I am not sure if there is a better way on how to encasuplate what's needed for knowing if the vector register is save. i.e. The fundamental issue here is the knowledge of the PPC vector register is in lib/Target/PowerPC directory, so whenever you need to know that knowledge from another directory (and in case, lib/CodeGen/AsmPrinter/AIXException.cpp), it's hard to pass the information in.

DiggerLin added inline comments.Jun 9 2021, 6:18 AM
llvm/include/llvm/CodeGen/AsmPrinter.h
755 ↗(On Diff #349907)

if we include the #include ../../Target/PowerPC/PPCMachineFunctionInfo.h in the AIXException.cpp , we have to implement the function getNumberOfVRSaved in file PPCMachineFunctionInfo.h , (not in the PPCMachineFunctionInfo.cpp) , otherwise it is link error when generate libLLVMAsmPrinter.a. implement a large function in PPCMachineFunctionInfo.h is not a good idea.

sfertile added inline comments.
llvm/include/llvm/CodeGen/AsmPrinter.h
755 ↗(On Diff #349907)

Why make it a member function at all, instead incorporate looping over to see if any of the CSR VRs are used in 'ShouldEmitEHBlock', and loop over and count the number of CSR VRs inline (or as a static helper with a MachineFunction argument) in PPCAIXAsmPrinter.cpp.

DiggerLin added inline comments.Jun 14 2021, 6:13 AM
llvm/include/llvm/CodeGen/AsmPrinter.h
755 ↗(On Diff #349907)

bool TargetLoweringObjectFileXCOFF::ShouldEmitEHBlock() is the directory of "llvm\lib\codegen" , it can not access the something like "PPC::V20" , which define in the headerfile of directory lib/Target/PowerPC" , not in the llvm/include directory

DiggerLin updated this revision to Diff 352235.Jun 15 2021, 1:38 PM

address comment

DiggerLin retitled this revision from [AIX] generate eh_info when vector registers are saved according to the traceback table. to [AIX][XCOFF] generate eh_info when vector registers are saved according to the traceback table..Jun 15 2021, 1:40 PM
jasonliu added inline comments.Jun 17 2021, 11:39 AM
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
2198 ↗(On Diff #352235)

nit:

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

I'm thinking if it's better to move this calculation into SetupMachineFunction.
And have a private data member (i.e. CurrentFnVRSavedNum) to record the result, so that we don't need to calculate it multiply times below.

1908

A comment above to explain why we want to emit exception info table in this condition would be useful.

jasonliu added inline comments.Jun 17 2021, 11:51 AM
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
2214 ↗(On Diff #352235)

In order for CodeGen to link successful, we would need AsmPrinter, but AsmPrinter already requires the CodeGen component for linking.
This could be a circular dependency.

DiggerLin marked an inline comment as done.Jun 17 2021, 1:54 PM
DiggerLin added inline comments.
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1885

there are only two places using the function, and there is not a lot workload of the function, I am prefer to keep as current implement, If you strong require me to implement as your suggestion , I can do as your suggestion.

DiggerLin updated this revision to Diff 352837.Jun 17 2021, 1:55 PM
DiggerLin marked an inline comment as done.

address comment

jasonliu added inline comments.Jun 21 2021, 8:48 AM
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1908–1909

clang-format the suggested comment.

I would also suggest to put a comment in AIXException::endFunction to indicate there is another place that would emit this eh info table structure. In case someone needs to update this structure, they would know they need to update in both places.

2088

nit

2199–2208

This variable could move to above line 2112, so that you only need to call it once in emitTracebackTable().

DiggerLin marked 3 inline comments as done.Jun 22 2021, 6:52 AM
DiggerLin added inline comments.
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1908–1909

thanks

DiggerLin updated this revision to Diff 353633.Jun 22 2021, 6:54 AM
DiggerLin marked an inline comment as done.

address comment

jasonliu accepted this revision.Jun 22 2021, 6:58 AM

LGTM. Thanks!

This revision is now accepted and ready to land.Jun 22 2021, 6:58 AM