Page MenuHomePhabricator

InstCombine: Restrict computeKnownBits() on everyValue to OptLevel > 2
ClosedPublic

Authored by MatzeB on Feb 2 2016, 6:14 PM.

Details

Summary

As part of r251146 InstCombine was extended to call computeKnownBits on
every value in the function to determine whether it happens to be
constant. This increases typical compiletime by 1-3% (5% in irgen+opt
time) in my measurements. On the other hand this case did not trigger
once in the whole llvm-testsuite.

This patch introduces the notion of ExpensiveCombines which are only
enabled for OptLevel > 2. I removed the check in InstructionSimplify as
that is called from various places where the OptLevel is not known but
given the rarity of the situation I think a check in InstCombine is
enough.

Diff Detail

Repository
rL LLVM

Event Timeline

MatzeB updated this revision to Diff 46732.Feb 2 2016, 6:14 PM
MatzeB retitled this revision from to InstCombine: Restrict computeKnownBits() on everyValue to OptLevel > 2.
MatzeB updated this object.
MatzeB added a reviewer: hfinkel.
MatzeB set the repository for this revision to rL LLVM.
MatzeB added subscribers: sanjoy, llvm-commits.

Two questions on the concept:

  • Are we sure that a boolean is enough? (per comparison to a "level", like Opt_level).
  • The trigger between expensive combine or not could (should?) be adjusted depending on where we stand in the pipeline (I mean not always turning it on or off depending on the opt level, but also depending on the position in the pipeline)

Given that I don't have a single file in the whole llvm-testsuite where this transformation kicks in I wouldn't know how to start tuning it, so this patch simply pushes it to -O3 only.

Given that I don't have a single file in the whole llvm-testsuite where this transformation kicks in I wouldn't know how to start tuning it, so this patch simply pushes it to -O3 only.

I'd rather us not do the expensive work at all instead of stick it under -O3. Sticking it behind a flag would mean that it gets tested less often. It doesn't seem to be pulling it's weight for it's cost...

hfinkel edited edge metadata.Feb 2 2016, 8:35 PM

Given that I don't have a single file in the whole llvm-testsuite where this transformation kicks in I wouldn't know how to start tuning it, so this patch simply pushes it to -O3 only.

I'd rather us not do the expensive work at all instead of stick it under -O3. Sticking it behind a flag would mean that it gets tested less often. It doesn't seem to be pulling it's weight for it's cost...

In the bigger picture, we really need to rework our known-bits analysis to have a cache. It is really silly that it is so expensive; but it's completely understandable given that it starts from scratch each time. I'm fairly certain that giving computeKnownBits an internal cache would also solve this problem.

In any case, the motivating use cases for this almost all involve (range) metadata or @llvm.assume. Maybe we could be smarter about when we do this based on whether such things are actually present in the function?

As we have this discussion on llvm-dev about compiletime at the moment: This is a big chunk we can get back, do we really want to block it on a rewrite of computeKnownBits() which I have no time for at the moment?

hfinkel accepted this revision.Mar 8 2016, 11:26 AM
hfinkel edited edge metadata.

As we have this discussion on llvm-dev about compiletime at the moment: This is a big chunk we can get back, do we really want to block it on a rewrite of computeKnownBits() which I have no time for at the moment?

Funny you should mention that ;) -- no, we don't. LGTM.

This revision is now accepted and ready to land.Mar 8 2016, 11:26 AM
This revision was automatically updated to reflect the committed changes.