This feature implements support for making entries in the exception section on XCOFF on the direct assembly path using the ".except" pseudo-op. It also provides functionality to lower entries (comprised of language and reason codes) into the exception section through the use of annotation metadata attached to llvm.ppc.trap/trapd/tw/tdw intrinsics. Integrated assembler support will be provided in another review. https://reviews.llvm.org/D133030 needs to merge first for LIT tests
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Add more reviewers as we add a target hook TargetLowering::LowerTargetIntrinsic in the target independent part while building target intrinsic.
llvm/include/llvm/CodeGen/TargetLowering.h | ||
---|---|---|
4616 | Please document the instruction and its arguments. Is Op necessary - should it be returned instead? Use SmallVectorImpl<SDValue>? | |
llvm/include/llvm/MC/MCStreamer.h | ||
639 | is it really necessary to add const to the integer args? | |
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp | ||
4868 | Do you need Result? Should this be returning anything? |
You should not add the integrated-as mode change to the existing non integrated-as change.
Better to post two patches for these two functionalities.
How would you recommend splitting these up? Beside the lit tests there is only one file (MCAsmStreamer) that has code specific to the direct assembly path, and only MCXCOFFStreamer and XCOFFObjectWriter are specific to the integrated-as path. The rest is shared code. My assumption was that if there is this much shared code it would be safer to make one review because the second review can't merge until this first one has and for a given feature, the full body of code that is needed for it is present in its review. That being said, do you prefer if I just move MCXCOFFStreamer, XCOFFObjectWriter and the object lit tests to a separate review?
Thanks, looks almost good to me. Some minor comments.
llvm/include/llvm/MC/MCObjectWriter.h | ||
---|---|---|
108 ↗ | (On Diff #461234) | This seems not belonging to this patch? |
llvm/lib/MC/MCAsmStreamer.cpp | ||
949 | format | |
llvm/lib/MC/MCStreamer.cpp | ||
1193 | Please use clang-format to reformat all the changes. The format here seems not right. | |
1197 | report_fatal_error seems like a better choice. llvm_unreachable may not stop the program in some configurations. | |
llvm/lib/MC/MCXCOFFStreamer.cpp | ||
89 | report_fatal_error | |
llvm/test/CodeGen/PowerPC/builtins-ppc-xlcompat-trap-annotations-64bit.ll | ||
12 | This seems not right. You already added RUN line for obj file type. You can just check: (can not redirect stderr to /dev/null) : OBJ: LLVM ERROR: emitXCOFFExceptDirective not yet supported for integrated assembler path. | |
llvm/test/CodeGen/PowerPC/builtins-ppc-xlcompat-trap-annotations.ll | ||
12 | Ditto. add a new runline for obj file type and check the error messages. |
llvm/lib/MC/MCStreamer.cpp | ||
---|---|---|
1199 | nit: format | |
llvm/lib/MC/MCXCOFFStreamer.cpp | ||
89 | nit: a space after "integrated" and reformat please | |
llvm/test/CodeGen/PowerPC/builtins-ppc-xlcompat-trap-annotations-64bit.ll | ||
2 | is the utils/update_llc_test_checks.py able to handle not --crash llc RUN line? I tested this, seems it is recognized as WARNING: Skipping non-llc RUN line: not --crash llc. If so, maybe we should delete this line as we are manually changing this file? | |
12 | nit: space between integrated and assembler | |
llvm/test/CodeGen/PowerPC/builtins-ppc-xlcompat-trap-annotations.ll | ||
2 | ditto. | |
3 | Seems like we are testing 32 bit targets in this file? If so, we should use 32 bit triple, like powerpc-unknown-linux-gnu powerpc-unknown-aix | |
14 | nit: space between integrated and assembler |
llvm/lib/MC/MCXCOFFStreamer.cpp | ||
---|---|---|
89 | Nit: Seems for a long string, the usual way is to put the space at the end of first line, instead of at the start of the second line. Need format here if the space is put at the end of integrated | |
llvm/test/CodeGen/PowerPC/builtins-ppc-xlcompat-trap-annotations.ll | ||
3 | hmm, there are still some lines testing 64 bit targets, like the first/fourth/fifth RUN lines. |
llvm/lib/Target/PowerPC/PPCISelLowering.cpp | ||
---|---|---|
16247 | This is always SDvalue()? If you're never going to create a node and just collect operands, I'd suggest you don't call this 'LowerTargetIntrinsic' and replace it with something more suitable (collectTargetIntrinsicOperands?) |
llvm/lib/Target/PowerPC/PPCISelLowering.cpp | ||
---|---|---|
16247 | The idea was to keep the hook somewhat open ended. This particular intrinsic does not require anything besides operand collection but in general another target intrinsic may require additional custom lowering steps. Since its unavoidable that this hook is called for every target intrinsic, shouldn't the hook be general enough that additional intrinsic cases can be added to it later? Let me know what you think. |
llvm/lib/Target/PowerPC/PPCISelLowering.cpp | ||
---|---|---|
16247 | I agree, but its caller doesn't use the return value at all. |
llvm/lib/Target/PowerPC/PPCISelLowering.cpp | ||
---|---|---|
16247 | Oh I see, the result gets overwritten anyway. In this case I suppose collectTargetIntrinsicOperands is the simplest solution. Will make the change shortly. |
llvm/lib/Target/PowerPC/PPCISelLowering.cpp | ||
---|---|---|
16247 | Also since the result is never used I'm assuming I should revert this function back to returning void? |
@shchenz I do not have commit privileges yet. If there are no further revision requests, could you commit this when you get the chance? Thanks
@shchenz I do not have commit privileges yet. If there are no further revision requests, could you commit this when you get the chance? Thanks
Sure. Will commit this soon.
Please document the instruction and its arguments. Is Op necessary - should it be returned instead? Use SmallVectorImpl<SDValue>?