This is an archive of the discontinued LLVM Phabricator instance.

[SimplifyCFG] allow speculation of exactly one expensive instruction (PR24818)
ClosedPublic

Authored by spatel on Dec 3 2015, 3:13 PM.

Details

Summary

This is the last general step to allow more IR-level speculation with a safety harness in place in CodeGenPrepare.

The intent is to restore the behavior enabled by:
http://reviews.llvm.org/rL228826

but prevent bad performance such as:
https://llvm.org/bugs/show_bug.cgi?id=24818

Earlier patches in this sequence:
D12882 (disable SimplifyCFG speculation for expensive instructions)
D13297 (have CGP despeculate expensive ops)
D14630 (have CGP despeculate special versions of cttz/ctlz)

As shown in the test cases, we only have two instructions currently affected: ctz for some x86 and fdiv generally. Allowing exactly one expensive instruction is a bit of a hack, but it lines up with what is currently implemented in CGP. If we make the despeculation more general in CGP, we can make the speculation here more liberal.

A follow-up patch will adjust the cost for sqrt and possibly other typically expensive math intrinsics (currently everything is cheap by default). GPU targets would likely want to override those expensive default costs (just as they probably should already override the cost of div/rem) because just about any math is cheaper than control-flow on those targets.

Diff Detail

Repository
rL LLVM

Event Timeline

spatel updated this revision to Diff 41806.Dec 3 2015, 3:13 PM
spatel retitled this revision from to [SimplifyCFG] allow speculation of exactly one expensive instruction (PR24818).
spatel updated this object.
spatel added reviewers: hfinkel, reames, jmolloy.
spatel added a subscriber: llvm-commits.
jmolloy requested changes to this revision.Dec 14 2015, 10:25 AM
jmolloy edited edge metadata.

Hi Sanjay,

Just a couple of comments.

James

lib/Transforms/Utils/SimplifyCFG.cpp
307 ↗(On Diff #41806)

I think !empty() is a quicker predicate than size(). I also think as Depth is an integer and you're using it in integer context, "Depth > 0" would be more explanatory.

I think this is the sort of heuristic that it would be good to get as a cl::opt.

This revision now requires changes to proceed.Dec 14 2015, 10:25 AM
spatel updated this revision to Diff 42778.Dec 14 2015, 2:27 PM
spatel edited edge metadata.
spatel marked an inline comment as done.

Patch updated based on James' feedback:

  1. Fixed check to use !empty() rather than size().
  2. Fixed check to specify 'Depth > 0' since it's an integer.
  3. Added cl::opt override to disable this.
spatel updated this revision to Diff 42783.Dec 14 2015, 2:48 PM
spatel edited edge metadata.

Patch updated:
The cl::opt check in the last rev was wrong. Added another run and checks to the fdiv test to make sure it's right this time.

jmolloy accepted this revision.Dec 15 2015, 8:31 AM
jmolloy edited edge metadata.

LGTM now, thanks!

This revision is now accepted and ready to land.Dec 15 2015, 8:31 AM
This revision was automatically updated to reflect the committed changes.