This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] XCOFF exception section support on the direct assembler path and exception language and reason code lowering from trap intrinsics
ClosedPublic

Authored by pscoro on Aug 18 2022, 10:00 AM.

Details

Summary

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

Diff Detail

Event Timeline

pscoro created this revision.Aug 18 2022, 10:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 18 2022, 10:00 AM
pscoro requested review of this revision.Aug 18 2022, 10:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 18 2022, 10:00 AM
pscoro retitled this revision from [PowerPC] XCOFF exception section support and exception language and reason code lowering from trap intrinsics to [PowerPC] XCOFF exception section support on assembler path and exception language and reason code lowering from trap intrinsics.Aug 18 2022, 10:06 AM
pscoro edited the summary of this revision. (Show Details)
pscoro added reviewers: shchenz, lei, rzurob.

Add more reviewers as we add a target hook TargetLowering::LowerTargetIntrinsic in the target independent part while building target intrinsic.

RKSimon added inline comments.Sep 9 2022, 5:44 AM
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?

pscoro updated this revision to Diff 459438.Sep 12 2022, 6:41 AM

Added integrated assembly support

pscoro updated this revision to Diff 459442.Sep 12 2022, 6:44 AM

formatting

pscoro updated this revision to Diff 459457.Sep 12 2022, 7:37 AM

small fix to lit test

pscoro retitled this revision from [PowerPC] XCOFF exception section support on assembler path and exception language and reason code lowering from trap intrinsics to [PowerPC] XCOFF exception section support on integrated and direct assembler paths and exception language and reason code lowering from trap intrinsics.Sep 12 2022, 7:41 AM
pscoro edited the summary of this revision. (Show Details)
pscoro added reviewers: stefanp, nemanjai.
pscoro updated this revision to Diff 459459.Sep 12 2022, 7:45 AM
pscoro edited the summary of this revision. (Show Details)

whitespace fix

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.

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.

+1 Please split these

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.

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.

+1 Please split these

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?

pscoro updated this revision to Diff 461230.Sep 19 2022, 9:20 AM

Split review

pscoro updated this revision to Diff 461234.Sep 19 2022, 9:31 AM

Added temporary llvm_unreachable for integrated assembler path

pscoro retitled this revision from [PowerPC] XCOFF exception section support on integrated and direct assembler paths and exception language and reason code lowering from trap intrinsics to [PowerPC] XCOFF exception section support ondirect assembler path and exception language and reason code lowering from trap intrinsics.Sep 19 2022, 9:56 AM
pscoro edited the summary of this revision. (Show Details)
pscoro retitled this revision from [PowerPC] XCOFF exception section support ondirect assembler path and exception language and reason code lowering from trap intrinsics to [PowerPC] XCOFF exception section support on the direct assembler path and exception language and reason code lowering from trap intrinsics.
pscoro edited the summary of this revision. (Show Details)Sep 19 2022, 9:59 AM

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
11 ↗(On Diff #461234)

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
11 ↗(On Diff #461234)

Ditto. add a new runline for obj file type and check the error messages.

pscoro marked 5 inline comments as done.Sep 21 2022, 10:59 AM
pscoro updated this revision to Diff 461978.Sep 21 2022, 12:05 PM

changes to LowerTargetIntrinsic

pscoro marked 2 inline comments as done.Sep 21 2022, 12:11 PM
shchenz added inline comments.Sep 21 2022, 7:32 PM
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
1 ↗(On Diff #461978)

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?

11 ↗(On Diff #461978)

nit: space between integrated and assembler

llvm/test/CodeGen/PowerPC/builtins-ppc-xlcompat-trap-annotations.ll
1 ↗(On Diff #461978)

ditto.

2 ↗(On Diff #461978)

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

13 ↗(On Diff #461978)

nit: space between integrated and assembler

pscoro updated this revision to Diff 462779.Sep 25 2022, 6:16 PM

addressing nits

pscoro marked 7 inline comments as done.Sep 25 2022, 6:19 PM
pscoro added inline comments.
llvm/lib/MC/MCXCOFFStreamer.cpp
89

ran clang-format after adding the space, no formatting was performed

llvm/test/CodeGen/PowerPC/builtins-ppc-xlcompat-trap-annotations.ll
2 ↗(On Diff #461978)

powerpc-unknown-aix already existed, I added powerpc-unknown-linux-gnu, no changes to assembly generated.

shchenz added inline comments.Sep 25 2022, 6:40 PM
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
2 ↗(On Diff #461978)

hmm, there are still some lines testing 64 bit targets, like the first/fourth/fifth RUN lines.

pscoro updated this revision to Diff 462791.Sep 25 2022, 7:22 PM
pscoro marked 2 inline comments as done.

changed lit test file names

shchenz accepted this revision.Sep 25 2022, 8:56 PM

LGTM. Thanks for your patience.

This revision is now accepted and ready to land.Sep 25 2022, 8:56 PM
RKSimon added inline comments.Sep 26 2022, 2:41 AM
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?)

pscoro marked an inline comment as done.Sep 26 2022, 6:06 AM
pscoro added inline comments.
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.

RKSimon added inline comments.Sep 26 2022, 6:23 AM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
16247

I agree, but its caller doesn't use the return value at all.

pscoro added inline comments.Sep 26 2022, 6:34 AM
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.

pscoro added inline comments.Sep 26 2022, 6:55 AM
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?

pscoro updated this revision to Diff 462902.Sep 26 2022, 7:29 AM

changed LowerTargetIntrinsic to CollectTargetIntrinsicOperands

pscoro marked an inline comment as done.Sep 26 2022, 7:31 AM
RKSimon accepted this revision.Sep 26 2022, 7:34 AM

Thanks, LGTM

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

This revision was landed with ongoing or failed builds.Sep 26 2022, 7:24 PM
This revision was automatically updated to reflect the committed changes.