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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76788