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

[Philip Reames via llvm-commits]

reames added inline comments.


================
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
----------------
sanjoy wrote:
> 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`.
I really think the assume/guard bits need to be there, but otherwise someone reading has no way to connect it to the implementation.  Adding a footnote with an reference might work.  Would you find that cleaner?


================
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
----------------
sanjoy wrote:
> 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.
> ```
> 
Hm, you repharsing of the missing context case is interesting.  In particular, we might be able to implement the semantics you suggest and get slightly more precise results than we do today.  This *isn't* our semantic today, but it would be a completely reasonable one and I don't think it would break any existing users.  

We'd have to pick beginning or end consistently.  End seems like it's more powerful, what do you think?  (i.e. for an invoke's normal edge, we'd want the context to be the first non-phi instruction in the normal block, not the invoke itself.)


================
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
----------------
sanjoy wrote:
> But what if execution does not reach the context instruction?
Today, I believe the answer is "don't do that!" or "UB".  This may very well be a latent bug in a couple of transforms.  JumpThreading - which is the main transform which exploits this - seems okay on first read.  


https://reviews.llvm.org/D25386