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

Mon Mar 16 06:26:26 PDT 2020

fhahn marked 3 inline comments as done.
fhahn added a comment.

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.

================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:6130
+
+      if (!isValidAssumeForContext(I, CtxI, nullptr))
+        continue;
----------------
nikic wrote:
> Should we also be passing the DT through?
> 
> Side note: https://github.com/llvm/llvm-project/blob/56aed6144a16cdd71e03e5855b19dab1fe9331e6/llvm/lib/Analysis/ValueTracking.cpp#L604-L609 needs to be updated to use the new block local dominance checks, instead of doing an instruction scan.
> Should we also be passing the DT through?

We certainly can. Maybe best done as a follow-up?

> Side note: https://github.com/llvm/llvm-project/blob/56aed6144a16cdd71e03e5855b19dab1fe9331e6/llvm/lib/Analysis/ValueTracking.cpp#L604-L609 needs to be updated to use the new block local dominance checks, instead of doing an instruction scan.

Agreed, I had the same though when looking at the code for isValudAssumeForContext. I've put it up at D76228.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76193