[PATCH] D11364: [CaptureTracker] Provide a numbered basic block to PointerMayBeCapturedBefore

[hfinkel at anl.gov]

hfinkel accepted this revision.
hfinkel added a comment.
This revision is now accepted and ready to land.

LGTM.


================
Comment at: include/llvm/Analysis/CaptureTracking.h:46
@@ -45,1 +45,3 @@
+  /// final parameter is true. An ordered basic block in \p OBB could be used
+  /// to speed up capture tracker queries.
   bool PointerMayBeCapturedBefore(const Value *V, bool ReturnCaptures,
----------------
capture tracker queries -> capture-tracker queries

================
Comment at: lib/Analysis/AliasAnalysis.cpp:347
@@ +346,3 @@
+/// in a BasicBlock. A ordered basic block \p OBB can be used to speed up
+/// instruction ordering queries inside the BasicBlock containing \p I.
+/// FIXME: this is really just shoring-up a deficiency in alias analysis.
----------------
instruction ordering queries -> instruction-ordering queries

================
Comment at: lib/Analysis/NumberedBasicBlock.cpp:71
@@ +70,3 @@
+  unsigned NB = NumberedInsts.lookup(B);
+  if (NA && NB)
+    return NA < NB;
----------------
bruno wrote:
> hfinkel wrote:
> > dberlin wrote:
> > > I have to admit i'm not a fan of treating the unsigneds like booleans values.
> > > If you are going to do this, can you use explicit compares or something?
> > > 
> > > Or at least, explain the logic of all of this?
> > > 
> > > It only works because lookup default constructs values :)
> > > 
> > > At the very least, i would add a comment as to what this block of code is doing, like
> > > 
> > > "First we lookup the instructions. If they don't exist, lookup will give us back zero.  If they both exist, we compare the numbers. Otherwise, if NA exists and NB doesn't, it means NA must come before NB because we would have numbered NB as well if it didn't.  The same is true for NB. If it exists, but NA does not, NA must come after it.
> > > 
> > > If neither exist, we need to number the block".
> > > 
> > > (Note also that the last line of code would make more sense if the function was comesBefore or something, or if it returned a boolean)
> > > Then it would be return comesBefore(A, B))
> > > Otherwise, if NA exists and NB doesn't, it means NA must come before NB because we would have numbered NB as well if it didn't. The same is true for NB. If it exists, but NA does not, NA must come after it.
> > 
> > Does this embed an assumption on the relative orderings of the calls to the function by the client. What if I start querying in the middle of the block somewhere using some previously-visited instruction and another I've not yet visited? I understand why CaptureTracking, which is following use/def chains walking from defs to uses, does not do this, but as a general utility, we either need to be more careful, or very carefully document the assumptions.
> > 
> If you're using some previously-visited instruction (call it A) and another you've not yet visited (B), it's just like Daniel explained in his comment; either B is already cached (when we first looked up A) if it was somewhere between BB.begin() and A, in this case it B comesBefore A. If B isn't cached, it means it must come after. Doesn't matter if you start querying in the middle of the block, if it's not in the map, it will search from BB.begin() (or resume from the last position it scanned) until it finds either A or B. Let me know if I didn't get your question right :-)
Indeed, I re-read the code and now I understand. Sorry for being bothersome about this. For some reason, I thought that find(A, B) might only number the instructions in between A and B. But it always numbers from the beginning until A or B.



http://reviews.llvm.org/D11364