Page MenuHomePhabricator

Introduce a @llvm.experimental.guard.on intrinsic
ClosedPublic

Authored by sanjoy on Mar 28 2016, 2:14 PM.

Details

Summary

As discussed on llvm-dev[1].

This change adds the basic boilerplate code around having this intrinsic
in LLVM:

  • Changes in Intrinsics.td, and the IR Verifier
  • A lowering pass to lower @llvm.experimental.guard.on to normal control flow
  • Inliner support

[1]: http://lists.llvm.org/pipermail/llvm-dev/2016-February/095523.html

Diff Detail

Repository
rL LLVM

Event Timeline

sanjoy updated this revision to Diff 51840.Mar 28 2016, 2:14 PM
sanjoy retitled this revision from to Introduce a @llvm.experimental.guard.on intrinsic.
sanjoy updated this object.
sanjoy added a subscriber: llvm-commits.
reames edited edge metadata.Mar 29 2016, 7:32 PM

A bunch of small comments. Looks close to ready to go in.

I think it would also help to layout a quick sketch of what comes next. How does this fit into the standard pipeline? What optimization passes are going to be changed? How does this interact with assume?

docs/LangRef.rst
12179 ↗(On Diff #51840)

Minor: I'd drop on from the name. The extra punctuation is distracting.

12201 ↗(On Diff #51840)

<args...>

Make a note about the var args part..

12202 ↗(On Diff #51840)

minor: instead of undef, maybe a volatile load from an unmodeled location?

12213 ↗(On Diff #51840)

wording: I wouldn't use deoptimize in this sense. It could be confused with policy choices like what to do with the code.

12215 ↗(On Diff #51840)

Mention widening explicitly as motivation for the guard.

lib/Transforms/Scalar/LowerGuardIntrinsic.cpp
11 ↗(On Diff #51840)

Clarify that after lowering, the guards can no longer be widened.

15 ↗(On Diff #51840)

Include sort.

39 ↗(On Diff #51840)

Possibly add a fastpath of the form:
if (getFunction(guard)->uses_empty())

return;

I'd like this to be on by default in the standard pipeline.

54 ↗(On Diff #51840)

I find this comment more confusing then helpful.

60 ↗(On Diff #51840)

Pulling out a helper function which inserts the new control flow, calling it, then dropping the original call might be clearer.

70 ↗(On Diff #51840)

possibly: "guarded"

73 ↗(On Diff #51840)

Two suggestions:

  • Use IRBuilder?
  • Explicitly branch on whether there's a return type rather than using two ternaries (one explicit, one implicit).
test/Transforms/LowerGuardIntrinsic/basic.ll
1 ↗(On Diff #51840)

Hm, it looks like the lowering pass is not part of the standard pipeline right now. I'd like to see this become part of the standard LLC pipeline. Doesn't have to be in this change specifically, but I will expect to see lowering tests showing the a guard gets lowered correctly all the way to assembly at some point.

5 ↗(On Diff #51840)

Why alwaysinline?

19 ↗(On Diff #51840)

Can you comment what each is testing?

sanjoy updated this revision to Diff 52113.Mar 30 2016, 1:31 PM
sanjoy marked 10 inline comments as done.
sanjoy edited edge metadata.
  • address review
docs/LangRef.rst
12202 ↗(On Diff #51840)

I think that'd be the opposite of what we want. "volatile load from an unmodeled location" => the optimizer needs to model it most conservatively (i.e. can be any value the programmer wants), "undef" => the optimizer can model it least conservatively (i.e. can be any value the optimizer wants).

lib/Transforms/Scalar/LowerGuardIntrinsic.cpp
15 ↗(On Diff #51840)

The convention in the other files (that include Scalar.h) seem to be to include Scalar.h and then have the rest of the list sorted.

39 ↗(On Diff #51840)

Done, but that's not enough to switch this on in the standard pipeline. As written, in the old pass manager, this will still break all analyses in the no-op case. We can consider fixing that, but not in this change.

54 ↗(On Diff #51840)

Removed.

test/Transforms/LowerGuardIntrinsic/basic.ll
1 ↗(On Diff #51840)

As mentioned earlier, adding this pass as is to the default pipeline is probably going to be a compile time regression.

5 ↗(On Diff #51840)

Leftover from where I copied the test from, removed.

19 ↗(On Diff #51840)

I didn't add comments, but renamed the functions to add some context.

reames accepted this revision.Mar 30 2016, 2:23 PM
reames edited edge metadata.

LGTM. This will probably evolve more as we explore how to actually use it, but doing that evolution in tree seems completely reasonable.

lib/Transforms/Scalar/LowerGuardIntrinsic.cpp
73 ↗(On Diff #52113)

Minor: also check uses.

This revision is now accepted and ready to land.Mar 30 2016, 2:23 PM
This revision was automatically updated to reflect the committed changes.