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

[Justin Lebar via llvm-commits]

I may be misreading, but it looks like that formats the entirety of
every file that's touched in the commit?  That violates my reading of
the LLVM style guide (don't do mass cleanups willy-nilly).

Anyway I just made a wrapper around "arc" that runs git-clang-format
(included in the llvm repo) on a commit range -- which only formats
lines you've touched -- whenever I run arc diff.  I even called my
wrapper "arc".  Enforcing formatting right before I upload the patch
works better for me than enforcing formatting every time I commit.

https://gist.github.com/anonymous/ce82f214ba78db50a288f12238d05897

On Thu, Apr 7, 2016 at 9:24 AM, Mehdi Amini <mehdi.amini at apple.com> wrote:
>
>> On Apr 7, 2016, at 9:15 AM, Justin Lebar <jlebar at google.com> wrote:
>>
>> jlebar marked 3 inline comments as done.
>>
>> ================
>> Comment at: lib/Transforms/Scalar/SpeculativeExecution.cpp:272
>> @@ +271,3 @@
>> +
>> +FunctionPass *createSpeculativeExecutionIfHasBranchDivergencePass() {
>> +  return new SpeculativeExecution(ONLY_IF_DIVERGENT_ARCH);
>> ----------------
>> chandlerc wrote:
>>> 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.
>>> I'd just go with [the pass a "named bool" arg] pattern as it seems like a lot less overhead for something simple and localized to this file.
>>
>> I think it's a question of the relative public-ness of APIs.  For the API that's private to this file, sure, I agree that an enum is overkill, I'll change it to a named bool.
>>
>> But for the public functions, it's a lot harder to guarantee that people are going to call them in a sane way.  In fact, people can call them from outside the LLVM project entirely.  So those are exactly the kinds of functions I was talking about in the blog post.  I also like separate named functions over a public enum in this case.
>>
>> (FWIW I don't think clang-tidy is relevant until it's run automatically as part of most peoples' workflows.  At the moment, I think I'm one of the only ones who runs clang-*format* automatically, and I had to write a wrapper around arc to accomplish that.
>
> A bit off topic (but may help still): I have a git hook that check that the code I am about to commit is clang-formatted, and if it fails reject the commit (add -n to override). Something along these lines: https://gist.github.com/wangkuiyi/7379a242f0d4089eaa75
>
> —
> Mehdi
>
>
>>  I haven't bothered to figure out clang-tidy, mostly because, eh, but also I think it's nontrivial because I'd need to give it -I paths.  If you actually want to rely on clang-tidy in general, we should figure out how to incorporate it into everyone's workflows; otherwise, I can't rely on other people using it.
>>
>> Moreover, the clang-tidy check I'm familiar with only ensures that, if you write the "named bool" comment in a certain way, the name is right.  It doesn't ensure that you name the bool at all, and it doesn't catch all of the creative ways you could write the name.  So even if it were running on every patch, it wouldn't prevent the problems described in that blog post.)
>>
>> Anyway, I think we're on the same page here.
>>
>>
>> http://reviews.llvm.org/D18625
>>
>>
>>
>