[PATCH] D38102: [XRay] support conditional return on PPC.

[Tim Shen via Phabricator via llvm-commits]

timshen added inline comments.


================
Comment at: llvm/lib/CodeGen/XRayInstrumentation.cpp:205
     // For the architectures which don't have a single return instruction
-    prependRetWithPatchableExit(MF, TII);
+    prependRetWithPatchableExit(MF, TII, {false, true});
+    break;
----------------
dberris wrote:
> 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.
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.


================
Comment at: llvm/test/CodeGen/PowerPC/xray-ret-is-terminator.ll:3
+
+define void @RetIsTerminator() #0 {
+; CHECK-LABEL @RetIsTerminator
----------------
dberris wrote:
> nit: Opportunity here for "IllBeBack". 😛 
GREAT CATCH! DONE! XD


https://reviews.llvm.org/D38102