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

[Nikita Popov via Phabricator via llvm-commits]

nikic marked an inline comment as done.
nikic 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 {
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76788