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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: lib/Analysis/LazyValueInfo.cpp:212
 
-    bool hasCachedValueInfo(Value *V, BasicBlock *BB) const {
-      if (isOverdefined(V, BB))
+    bool getCachedValueInfo(ValueLatticeElement &BBLV, Value *V,
+                            BasicBlock *BB) const {
----------------
nikic wrote:
> fhahn wrote:
> > nikic wrote:
> > > fhahn wrote:
> > > > Would it make sense to return Optional<ValuelLatticeElement> instead? IMO that would make things a bit more explicit (and personally I think it's easier to follow than using 'out' parameters)
> > > I'm a bit fan of Option(al) ... in Rust. Unfortunately, the ergonomics of Optional in C++ really leave something to be desired :(
> > > 
> > > LVI is mostly based around the out parameter + bool return pattern, with only one part using Optional returns: https://github.com/llvm/llvm-project/blob/9e1455dc236252a60066c312c6d2e8a7ed66f609/llvm/lib/Analysis/LazyValueInfo.cpp#L1047-L1051
> > > 
> > > This kind of code is (imho) rather ugly and inefficient (in this example, it will result in an entirely unnecessary copy of the range).
> > > 
> > > Possibly I'm just missing some kind of standard pattern for this? I wasn't able to find anything on this, not even in C++17.
> > > 
> > > Another possibility would be to add another state to `ValueLatticeElement` like `unresolved` for this purpose. I tried using `unknown` for this initially, because I thought it would not occur inside the LVI cache, but it does occur for values coming from unreachable blocks.
> > > I'm a bit fan of Option(al) ... in Rust. Unfortunately, the ergonomics of Optional in C++ really leave something to be desired :(
> > 
> > Unfortunately there's no direct equivalent for Rust's `if let Some(m) = &MaybeFoo {}` which lets you unpack the value from the optional, if it has one as far as I am aware. Probably the next best thing would be to use `if (Optional<ConstantRange> CR = getOptionalCR()) {` and use `*CR`/`CR.getValue()` in the if body. Using auto in the if that's just one character more :)
> > 
> > > 
> > > This kind of code is (imho) rather ugly and inefficient (in this example, it will result in an entirely unnecessary copy of the range).
> > > 
> > 
> > Are you referring to the the explicit copy `ConstantRange LHSRange = LHSRes.getValue();`? That's not necessary, `getValue` returns a reference. There's the copy/move of the optional result from callee to caller of course, but ideally the compiler should be able to remove them I guess, as long as it can inline things. In some cases, it might even save some work, as you do not have to unconditionally construct a ValueLatticeElement in the caller.
> > 
> > > Possibly I'm just missing some kind of standard pattern for this? I wasn't able to find anything on this, not even in C++17.
> > 
> > I don't think there is much potential for improvement here, as it is just a regular type and the `if (auto X = ..)` syntax relies on the bool operator of the result :(
> > 
> > > LVI is mostly based around the out parameter + bool return pattern, with only one part using Optional returns: https://github.com/llvm/llvm-project/blob/9e1455dc236252a60066c312c6d2e8a7ed66f609/llvm/lib/Analysis/LazyValueInfo.cpp#L1047-L1051
> > 
> > Right. Conceptually it seems Optional would be a good fit here, but if the existing code uses the other pattern the incentive for change is not that big I guess.
> I just gave the Optional conversion a try, and it looks a lot nicer than I expected: https://gist.github.com/nikic/f437fc7f667e433f462232aeb5272bc1
> 
> I'd like to apply this as a NFC cleanup after this patch though, as has to touch a lot more code.
SGTM, thanks!


================
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);
----------------
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?


================
Comment at: lib/Analysis/LazyValueInfo.cpp:1523
-    // No new information.
-    Result = LocalResult;
-    return true;
----------------
This case seems to be gone in the new code? Not sure if it matters much in practice?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76788