This is an archive of the discontinued LLVM Phabricator instance.

[ConstantRange] Add setSizeSmallerThanOf method.
ClosedPublic

Authored by mzolotukhin on Mar 17 2017, 2:15 PM.

Details

Summary

ConstantRange class currently has a method getSetSize, which is mostly used to
compare set sizes of two constant ranges (there is only one spot where it's used
in a slightly different scenario). This patch introduces setSizeSmallerThanOf
method, which does such comparison in a more efficient way. In the original
method we have to extend our types to (BitWidth+1), which can result it using
slow case of APInt, extra memory allocations, etc.

The change is supposed to not change any functionality, but it slightly improves
compile time. Here is compile time improvements that I observed on CTMark:

  • tramp3d-v4 -2.02%
  • pairlocalalign -1.82%
  • lencod -1.67%

Diff Detail

Repository
rL LLVM

Event Timeline

mzolotukhin created this revision.Mar 17 2017, 2:15 PM
sanjoy accepted this revision.Mar 17 2017, 2:23 PM

Nice catch! LGTM.

include/llvm/IR/ConstantRange.h
187 ↗(On Diff #92204)

Comment?

lib/IR/ConstantRange.cpp
276 ↗(On Diff #92204)

How about calling this isStrictlySmallerThan?

Also, please add a comment on why you're not simply using getSetSize here. Otherwise a later "cleanup change" may break your optimization.

This revision is now accepted and ready to land.Mar 17 2017, 2:23 PM
sanjoy added inline comments.Mar 17 2017, 2:26 PM
lib/IR/ConstantRange.cpp
276 ↗(On Diff #92204)

Actually, "smaller than" can be misinterpreted.

How about isSizeStrictlySmallerThanOf?

I'm mainly trying to address two things:

  • Since this is a predicate, I'd rather have its name start with is
  • The set in the beginning is redundant, IMO
  • Address review remarks.
This revision was automatically updated to reflect the committed changes.

Thanks for the review, Sanjoy!

A follow-up question: do we want to keep getSetSize? It's currently only used in SimplifyCFG.cpp:

if (Span.getSetSize().ugt(8) || Span.isEmptySet())

which can be replaced with something like if (!Span.isSizeStrictlySmallerThanOf(ConstantRange(8, /*full=*/ true)) || Span.isEmptySet()).

Thanks for the review, Sanjoy!

A follow-up question: do we want to keep getSetSize? It's currently only used in SimplifyCFG.cpp:

if (Span.getSetSize().ugt(8) || Span.isEmptySet())

which can be replaced with something like if (!Span.isSizeStrictlySmallerThanOf(ConstantRange(8, /*full=*/ true)) || Span.isEmptySet()).

Won't you need !Span.isSizeStrictlySmallerThanOf(ConstantRange(3, /*full=*/ true))?

I'd actually propose adding a isSizeStrictlySmallerThan that takes an APInt, and calling it with 8.

Thanks for the review, Sanjoy!

A follow-up question: do we want to keep getSetSize? It's currently only used in SimplifyCFG.cpp:

if (Span.getSetSize().ugt(8) || Span.isEmptySet())

which can be replaced with something like if (!Span.isSizeStrictlySmallerThanOf(ConstantRange(8, /*full=*/ true)) || Span.isEmptySet()).

I'd rather have a isSizeStrictlySmallerThan that takes a set size directly.