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

Sun Apr 5 07:27:37 PDT 2020

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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76788