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

Mon Mar 16 01:21:43 PDT 2020

nikic added a comment.

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>.

================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:6130
+
+      if (!isValidAssumeForContext(I, CtxI, nullptr))
+        continue;
----------------
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.

================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:6138
+      ConstantRange RHS =
+          computeConstantRange(Cmp->getOperand(1), UseInstrInfo);
+      CR = CR.intersectWith(
----------------
We may need to start passing a Depth parameter through, as this is now recursive. Though, as you don't pass through AC/CtxI here, recursion is implicitly limited to two levels.

================
Comment at: llvm/unittests/Analysis/ValueTrackingTest.cpp:1056
+    EXPECT_EQ(100, CR2.getUpper());
+  }
+}
----------------
Possibly worth checking a contradicting assumption? Which would result in an empty range.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76193