[PATCH] D76193: [ValueTracking] Use assumptions in computeConstantRange.

Tue Mar 17 16:14:05 PDT 2020

nikic accepted this revision.
nikic added a comment.
This revision is now accepted and ready to land.

In D76193#1924399 <https://reviews.llvm.org/D76193#1924399>, @fhahn wrote:

> In D76193#1923931 <https://reviews.llvm.org/D76193#1923931>, @nikic wrote:
>
> > Thanks for providing the context, clearly LVI can't help there...
> >
> > This looks fine to me technically, only concern is whether this carries its weight in compile time.
>
>
>
>
> > Together with the linked change, this is a 0.5% regression <http://llvm-compile-time-tracker.com/compare.php?from=3ffb5ef7b030b239df74e9885d96634ba7df92eb&to=1eb8a2c166578abfef1c1b50e8e66474158cfb5b&stat=instructions>.
>
> That's a neat site. It looks very useful. Do you know if there's any more information about the site?  Is there a way to also check the patches together with D76228 <https://reviews.llvm.org/D76228>?
>
> With respect to the regression, I am a bit surprised that there are seem to be quite a few assumptions encoded using assume() from clang. I'll check.

If you push a branch that starts with `perf/` to your fork <https://github.com/fhahn/llvm-project>, it should get picked up. I've also done a run with the additional patch: Individual commits <http://llvm-compile-time-tracker.com/index.php?branch=nikic%2Fperf%2Ffhahn-basicaa-constant-range-2> and all three combined <http://llvm-compile-time-tracker.com/compare.php?from=b9f1b8be1cb02f6159c27856e33996a7edb2bd18&to=18d82b328cc00f9cb3102a62cb61ac66272f5e75&stat=instructions>.

Turns out the change to isValidAssumeForContext() in D76228 <https://reviews.llvm.org/D76228> is neutral, I guess we don't exercise that code path much. This patch itself is a slight regression (the function is called often enough that the extra params and checks make a measurable difference). The BasicAA patch is a bit larger regression. I doubt this has anything to do with the assume handling, it's probably just the cost of the extra computeConstantRange() calls in a common analysis.

Anyway, this patch looks fine to me technically, so LGTM.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76193/new/

https://reviews.llvm.org/D76193