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

[Eric Christopher via Phabricator via llvm-commits]

echristo accepted this revision.
echristo added a comment.

Other than the readability issue, sgtm.

-eric



================
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;
----------------
timshen wrote:
> 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.
Bools are pretty terrible and while I agree that designated initializers would be better, named constants are at least better than false, true.


================
Comment at: llvm/lib/CodeGen/XRayInstrumentation.cpp:45
+
+  InstrumentationOptions(bool HandleTailcall, bool HandleAllReturns)
+      : HandleTailcall(HandleTailcall), HandleAllReturns(HandleAllReturns) {}
----------------
How about an enum mask set?


https://reviews.llvm.org/D38102