[PATCH] D76788: [LVI] Cleanup/unify cache access

[Nikita Popov via Phabricator via llvm-commits]

nikic marked 2 inline comments as done.
nikic added inline comments.


================
Comment at: lib/Analysis/LazyValueInfo.cpp:1000
-  ConstantRange Range = ConstantRange::getFull(OperandBitWidth);
-  if (hasBlockValue(I->getOperand(Op), BB)) {
-    ValueLatticeElement Val = getBlockValue(I->getOperand(Op), BB);
----------------
fhahn wrote:
> It looks like the original code checks for the block value again after pushing it. I think the new code skips the check. Not sure if it actually matters though?
`pushBlockValue()` does not change the result of `hasBlockValue()` (it pushes to the work stack, but does not modify the cache). The duplicate `hasBlockValue()` call here was to handle the case where `pushBlockValue()` returned false (and thus did not early exit above).

There is indeed a change in behavior here, this is one of the "not quite NFC" parts mentioned in the summary: Previously the case where the value is assumed overdefined due to a cycle would always result in a full range here, while now it will be treated like other overdefined values by trying to perform the assume intersect first.


================
Comment at: lib/Analysis/LazyValueInfo.cpp:1523
-    // No new information.
-    Result = LocalResult;
-    return true;
----------------
fhahn wrote:
> This case seems to be gone in the new code? Not sure if it matters much in practice?
Right, this is the second "not quite NFC" part. If we assume an overdefined block value due to a cycle, this previously did not perform the assume intersections below, while now it does.

In both cases, this may improve analysis results in some edge cases, though I doubt it happens in practice. It could also make analysis more expensive, but at least I did not see any compile-time regressions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76788