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.
Details
Diff Detail
- Build Status
Buildable 10516 Build 10516: arc lint + arc unit
Event Timeline
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 | nit: Probably name this as InstrumentationOptions? | |
39 | Please provide a default value here, say bool HandleTailCall = true;? |
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.
Still LGTM, with one last suggestion.
llvm/lib/CodeGen/XRayInstrumentation.cpp | ||
---|---|---|
205 | 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 | ||
4 | nit: Opportunity here for "IllBeBack". ๐ |
Updated test.
llvm/lib/CodeGen/XRayInstrumentation.cpp | ||
---|---|---|
205 | 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 | ||
4 | GREAT CATCH! DONE! XD |
nit: Probably name this as InstrumentationOptions?