[PATCH] D25386: [LVI] Try to document the complexities of context instructions

[Sanjoy Das via llvm-commits]

sanjoy added inline comments.


================
Comment at: include/llvm/Analysis/LazyValueInfo.h:68
+  /// Understanding Context Instructions, Assumes, and Guards
+  /// --------------------------------------------------------
+  ///    The LVI query predicates accept a CFG element (Block, Edge) and an
----------------
I almost hate nitpicking on this, but you have an extra `~`.


================
Comment at: include/llvm/Analysis/LazyValueInfo.h:69
+  /// --------------------------------------------------------
+  ///    The LVI query predicates accept a CFG element (Block, Edge) and an
+  /// optional context instruction.  Without a context specified, the return
----------------
I'd s/`Block, Edge`/`basic block or basic block edge`/  The way you've written it now, it looks like a tuple of a block and an edge.


================
Comment at: include/llvm/Analysis/LazyValueInfo.h:73
+  /// element.  That is, a value might be known constant within a block or on a
+  /// given edge.  Without a context, the facts returned a true anywhere within
+  /// the specified block.
----------------
s/a true/are true/


================
Comment at: include/llvm/Analysis/LazyValueInfo.h:78
+  /// which throws if it's argument is non-zero.  For an context before that
+  /// function call, we have to assume the value might be non-zero, but after
+  /// the function call, we know the value is zero.  In general, we don't
----------------
This is stylistic and my personal preference (so feel free to ignore), but I'd not put the stuff about IPO / guards / assumes here (maybe as a footnote, but not "inlined" here like this).

Instead, I'd say `However, there are control flow facts which can become true part way through a block (for instance, once after a pointer has been dereferenced, we know it is non-null).` and continue with your main point of describing `CtxI`.


================
Comment at: include/llvm/Analysis/LazyValueInfo.h:84
+  /// special intrinsics with related semantics: assume, and guards.
+  ///    The optional context instruction provides a means to query these
+  /// facts.  We also allow context instructions which are not within the block
----------------
Have you tried restructuring the description this way:

```
The LVI query predicates accept a CFG element (Block, Edge) and an optional context instruction.  When a context instruction is present ... (semantics of the context instruction).  If it isn't present, we pretend the context instruction is the beginning of the block / somewhere "inside" the edge.
```



================
Comment at: include/llvm/Analysis/LazyValueInfo.h:91
+  /// property of the specified Value which is true at the given CFG element if
+  /// execution also reaches the context instruction.
+  ///    TODO: We could be much more aggressive about exploiting the last point
----------------
But what if execution does not reach the context instruction?


https://reviews.llvm.org/D25386