[PATCH] D18625: [Speculation] Add a SpeculativeExecution mode where the pass does nothing unless TTI::hasBranchDivergence() is true.

[Chandler Carruth via llvm-commits]

chandlerc requested changes to this revision.
chandlerc added a reviewer: chandlerc.
chandlerc added a comment.
This revision now requires changes to proceed.

Two high level comment:

1. I'd just add in the PassManagerBuilder change here...

2. Please add a pass-local flag that allows you to test the pass in both modes, and some test cases exercising the behavior here.


================
Comment at: lib/Transforms/Scalar/SpeculativeExecution.cpp:96
@@ -86,1 +95,3 @@
 namespace {
+enum ExecutionMode { ALWAYS, ONLY_IF_DIVERGENT_ARCH };
+
----------------
LLVM doesn't use this style for enums.

However, I wouldn't use an enum here. I would just use a well named boolean.

================
Comment at: lib/Transforms/Scalar/SpeculativeExecution.cpp:272
@@ +271,3 @@
+
+FunctionPass *createSpeculativeExecutionIfHasBranchDivergencePass() {
+  return new SpeculativeExecution(ONLY_IF_DIVERGENT_ARCH);
----------------
joker.eph wrote:
> jlebar wrote:
> > tra wrote:
> > > Could we pass an argument to createSpeculativeExecutionPass() instead of encoding it in the name?
> > We could, although I wouldn't want it to be a boolean; see my (by now quite old) rant about this: http://jlebar.com/2011/12/16/Boolean_parameters_to_API_functions_considered_harmful..html
> > 
> > I figured it wasn't worth exposing an enum into the llvm namespace, but if that's preferred, I can do it.
> Offtopic: I'm so glad to see your blog post entry. I'd had the same argument on a patch review once on llvm-commits! I'm trying to avoid it as much as possible.
While I'm sympathetic, and I think having separate public functions is fine, I wouldn't reach to the full power of an enum here.

Within LLVM and Clang we pretty commonly will use '/*MyFlagName*/ true' as the argument. There is even a clang-tidy check that will ensure the name in the argument and the name in the parameter match so you don't get the ugly bugs here. I'd just go with that pattern as it seems like a lot less overhead for something simple and localized to this file.


http://reviews.llvm.org/D18625