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

[Bruno Cardoso Lopes]

bruno added a comment.

Thanks Hal and Daniel for the review, really appreciated your feedback.


================
Comment at: include/llvm/Analysis/NumberedBasicBlock.h:34
@@ +33,3 @@
+
+class NumberedBasicBlock {
+private:
----------------
dberlin wrote:
> Given the description of what this class does, why is it called numbered anything?
> Shouldn't it be "OrderedBasicBlock"?
> 
> You are just describing and querying things about the order
> The fact that you maintain the order using numbering is an implementation detail (and in fact i've seen proposals to change how BB's are laid out so that ordering would not require numbering.)
Right. Done!

================
Comment at: include/llvm/Analysis/NumberedBasicBlock.h:43
@@ +42,3 @@
+  /// instruction while walking 'BB'.
+  const Instruction *find(const Instruction *A, const Instruction *B);
+
----------------
dberlin wrote:
> The implementation here seems confusing.  Find doesn't find anything, instead, it is really trying to give a before relationship (IE it is literally answering the dominates query)
> 
> If you want to return the instruction, i'd call this "first_of" or something.
> But i'm not clear on why it returns an instruction, instead of being called "comesBefore" and returning a boolean.
> 
> All returning the instruction does is make the logic of the caller more confusing :)
> 
comesBefore + retuning bool seems like a better approach indeed!

================
Comment at: lib/Analysis/CaptureTracking.cpp:196
@@ -250,1 +195,3 @@
 
+  if (!NBB) {
+    NumberedBasicBlock NewNBB(I->getParent());
----------------
dberlin wrote:
> Errr, it looks like this duplicates the logic below, just to avoid the construction cost of the object all the time?
> 
> If so, is there a reason you don't just new and delete one?
OK

================
Comment at: lib/Analysis/NumberedBasicBlock.cpp:70
@@ +69,3 @@
+  unsigned NA = NumberedInsts.lookup(A);
+  unsigned NB = NumberedInsts.lookup(B);
+  if (NA && NB)
----------------
hfinkel wrote:
> So it seems you start numbering from 1 (that should be in a comment somewhere). Then, for every instruction you've not already visited you insert an implicit 0 entry in the map for it. You then replace this entry with another number when you call find. I'd rather we not insert implicit entries, instead, just test the iterators.
> 
>   auto NAI = NumberedInsts.find(A);
>   auto NBI = NumberedInsts.find(B);
> 
>   if (NAI != NumberedInsts.end() && NBI != NumberedInsts.end()) {
>     ...
> 
> with that done, you can start numbering from 0, which I think makes more sense in general.
> 
OK

================
Comment at: lib/Analysis/NumberedBasicBlock.cpp:71
@@ +70,3 @@
+  unsigned NB = NumberedInsts.lookup(B);
+  if (NA && NB)
+    return NA < NB;
----------------
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 :-)


http://reviews.llvm.org/D11364