This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by jlebar on Mar 30 2016, 2:45 PM.

Details

Summary

This lets us add this pass to the IR pass manager unconditionally; it
will simply not do anything on targets without branch divergence.

Diff Detail

Event Timeline

jlebar updated this revision to Diff 52130.Mar 30 2016, 2:45 PM
jlebar retitled this revision from to [Speculation] Add a SpeculativeExecution mode where the pass does nothing unless TTI::hasBranchDivergence() is true..
jlebar updated this object.
jlebar added a reviewer: tra.
jlebar added subscribers: chandlerc, rnk, jingyue, llvm-commits.
tra added inline comments.Mar 30 2016, 3:12 PM
lib/Transforms/Scalar/SpeculativeExecution.cpp
272

Could we pass an argument to createSpeculativeExecutionPass() instead of encoding it in the name?

jlebar added inline comments.Mar 30 2016, 3:14 PM
lib/Transforms/Scalar/SpeculativeExecution.cpp
272

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.

mehdi_amini added inline comments.
lib/Transforms/Scalar/SpeculativeExecution.cpp
272

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.

chandlerc requested changes to this revision.Apr 7 2016, 12:32 AM
chandlerc added a reviewer: chandlerc.

Two high level comment:

  1. I'd just add in the PassManagerBuilder change here...
  1. Please add a pass-local flag that allows you to test the pass in both modes, and some test cases exercising the behavior here.
lib/Transforms/Scalar/SpeculativeExecution.cpp
96

LLVM doesn't use this style for enums.

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

272

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.

This revision now requires changes to proceed.Apr 7 2016, 12:32 AM
jlebar marked 3 inline comments as done.Apr 7 2016, 9:15 AM
jlebar added inline comments.
lib/Transforms/Scalar/SpeculativeExecution.cpp
272

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. 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.

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

Chandler, how would you feel if I added a test to D18626 instead? (The test would check that we do the speculative execution only on the appropriate targets.)

Adding a flag to force speculative execution on (or off?) doesn't seem to test what we're actually after, namely that we do SpeculativeExecution iff the target has divergent branches.

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

Chandler, how would you feel if I added a test to D18626 instead? (The test would check that we do the speculative execution only on the appropriate targets.)

You should also probably do some testing there, but...

Adding a flag to force speculative execution on (or off?) doesn't seem to test what we're actually after, namely that we do SpeculativeExecution iff the target has divergent branches.

I'm not suggesting you force speculation on or off here. I'm suggesting you toggle the two modes described in the comments: "always" vs "only if divergent arch". Essentially, a flag that simulates the two public functions you can call to construct the pass.

You can use a target that will trivially never have speculation enabled to ensure you can observe this.

I'm essentially saying you should test two different things:

  1. The pass has an "always" mode that doesn't care about the target. That should involve a flag here.
  1. Targets can differentially control this when not in the "always" mode. That can involve having a flag for a single target that swings it both ways, or a flag that swings all targets, or just comparing two known targets. However, the last of these makes it annoying because we have to disable the test if *either* of the targets are missing.

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

Chandler, how would you feel if I added a test to D18626 instead? (The test would check that we do the speculative execution only on the appropriate targets.)

You should also probably do some testing there, but...

Adding a flag to force speculative execution on (or off?) doesn't seem to test what we're actually after, namely that we do SpeculativeExecution iff the target has divergent branches.

I'm not suggesting you force speculation on or off here. I'm suggesting you toggle the two modes described in the comments: "always" vs "only if divergent arch". Essentially, a flag that simulates the two public functions you can call to construct the pass.

You can use a target that will trivially never have speculation enabled to ensure you can observe this.

I'm essentially saying you should test two different things:

  1. The pass has an "always" mode that doesn't care about the target. That should involve a flag here.
  1. Targets can differentially control this when not in the "always" mode. That can involve having a flag for a single target that swings it both ways, or a flag that swings all targets, or just comparing two known targets. However, the last of these makes it annoying because we have to disable the test if *either* of the targets are missing.
jlebar updated this revision to Diff 53455.Apr 12 2016, 1:47 PM
jlebar edited edge metadata.

Add test, and flag for testing.

OK, this what you had in mind?

chandlerc accepted this revision.Apr 12 2016, 2:48 PM
chandlerc edited edge metadata.

Just some minor nits and tweaks to the testing below. Thanks. LGTM, feel free to submit with this stuff addressed, or ask for clarifications if any of it doesn't make sense.

lib/Transforms/Scalar/SpeculativeExecution.cpp
101–103

I would use either ...Arch or ...Target consistently.

Does it make sense to make the flag be the default argument rather than an ||? I don't feel strongly either way.

test/Transforms/SpeculativeExecution/divergent-target.ll
7 ↗(On Diff #53455)

Maybe put this in '-mtriple' arguments to the two target specific ones? That way the "always" mode isn't tied to this target?

It would also make the '-march' above more clean as it could be a parallel '-mtriple=x86_64-...'

11 ↗(On Diff #53455)

This looks stale?

This revision is now accepted and ready to land.Apr 12 2016, 2:48 PM
jlebar updated this revision to Diff 53471.Apr 12 2016, 3:06 PM
jlebar marked 2 inline comments as done.
jlebar edited edge metadata.

Address Chandler's comments.

Thank you for the review, Chandler!

test/Transforms/SpeculativeExecution/divergent-target.ll
7 ↗(On Diff #53455)

Much better, thanks. I'd been trying -mcpu and -march, but that didn't work, and I didn't look closely enough at opt.cpp to see that -mtriple would do the same.

This revision was automatically updated to reflect the committed changes.