This is an archive of the discontinued LLVM Phabricator instance.

[XRay] support conditional return on PPC.
ClosedPublic

Authored by timshen on Sep 20 2017, 2:41 PM.

Details

Summary

Conditional returns were not taken into consideration at all. Implement them by turning them into jumps and normal returns. This means there is a slightly higher performance penalty for conditional returns, but this is the best we can do, and it still disturbs little of the rest.

Diff Detail

Repository
rL LLVM

Event Timeline

timshen created this revision.Sep 20 2017, 2:41 PM
dberris accepted this revision.Sep 20 2017, 5:06 PM

LGTM for the instrumentation pass side (with a couple of suggestions).

I'll let @echristo decide on the AsmPrinter and MCInstLower implementations.

llvm/lib/CodeGen/XRayInstrumentation.cpp
37 ↗(On Diff #116077)

nit: Probably name this as InstrumentationOptions?

39 ↗(On Diff #116077)

Please provide a default value here, say bool HandleTailCall = true;?

This revision is now accepted and ready to land.Sep 20 2017, 5:06 PM
timshen updated this revision to Diff 116267.Sep 21 2017, 2:22 PM

s/Option/InstrumentationOptions/

Also updated AsmPrinter, to print the original instruction if the opcode is not supported.

Also add isTerminator to PATCHABLE_RET, otherwise AsmPrinter may not print some of the jump labels.

timshen updated this revision to Diff 116268.Sep 21 2017, 2:26 PM

Disable default ctor for InstrumentationOptions.

timshen added inline comments.Sep 21 2017, 2:26 PM
llvm/lib/CodeGen/XRayInstrumentation.cpp
37 ↗(On Diff #116077)

Done.

39 ↗(On Diff #116077)

I actually prefer not to, as the intention is to force the user to think about the choice to make. Providing default values make the user think less about the behaviors.

I disabled default constructor.

dberris accepted this revision.Sep 21 2017, 2:37 PM

Still LGTM, with one last suggestion.

llvm/lib/CodeGen/XRayInstrumentation.cpp
205 ↗(On Diff #116269)

Looking at this, I'm inclined to suggest that using an enum class for spelling out whether to handle tail calls might be more readable. Right now I have to see what the meaning of 'false' in the first position is. Something like:

prependRetWithPatchableExit(MF, TII, {InstrumentationOptions::TailCalls::IGNORE, InstrumentationOptions::Returns::HANDLE_ALL});

The enum class may or may not be nested in InstrumentationOptions, so I'll leave that up to you.

llvm/test/CodeGen/PowerPC/xray-ret-is-terminator.ll
3 ↗(On Diff #116269)

nit: Opportunity here for "IllBeBack". 😛

timshen updated this revision to Diff 116283.Sep 21 2017, 3:30 PM
timshen marked an inline comment as done.

Updated test.

llvm/lib/CodeGen/XRayInstrumentation.cpp
205 ↗(On Diff #116269)

FYI designated initializer (http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/p0329r4.pdf) should be in C++20, so ultimately we'll have this:

prependRetWithPatchableExit(MF, TII, {.HandleTailCalls = false, .HandleAllReturns = true});

but not now. :)

As for your suggested solution, I feel like the callee side code will be much longer and confusing, so I'll keep it as is.

llvm/test/CodeGen/PowerPC/xray-ret-is-terminator.ll
3 ↗(On Diff #116269)

GREAT CATCH! DONE! XD

echristo accepted this revision.Sep 21 2017, 5:45 PM

Other than the readability issue, sgtm.

-eric

llvm/lib/CodeGen/XRayInstrumentation.cpp
45 ↗(On Diff #116283)

How about an enum mask set?

205 ↗(On Diff #116269)

Bools are pretty terrible and while I agree that designated initializers would be better, named constants are at least better than false, true.

This revision was automatically updated to reflect the committed changes.