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

[hfinkel at anl.gov]

hfinkel added a comment.

> In the same testcase used in r240560, compile time goes down from 2min to 30s.


Nice!


================
Comment at: include/llvm/Analysis/NumberedBasicBlock.h:10
@@ +9,3 @@
+//
+// This file defines NumberedBasicBlock class. NumberedBasicBlock maintains an
+// interface where clients can query if one instruction comes before another
----------------
defines NumberedBasicBlock class -> defines the NumberedBasicBlock class

================
Comment at: include/llvm/Analysis/NumberedBasicBlock.h:14
@@ +13,3 @@
+// relative position between instructions one can use NumberedBasicBlock to do
+// such queries. NumberedBasicBlock lazily built after a source BasicBlock and
+// maintains an internal Instruction -> Position map. A NumberedBasicBlock
----------------
NumberedBasicBlock lazily built after -> NumberedBasicBlock is lazily built [on|off of|around]

================
Comment at: lib/Analysis/NumberedBasicBlock.cpp:15
@@ +14,3 @@
+// way to query relative position between instructions one can use
+// NumberedBasicBlock to do such queries. NumberedBasicBlock lazily built after
+// a source BasicBlock and maintains an internal Instruction -> Position map. A
----------------
NumberedBasicBlock lazily built after -> NumberedBasicBlock is lazily built [on|off of|around]

================
Comment at: lib/Analysis/NumberedBasicBlock.cpp:37
@@ +36,3 @@
+const Instruction *NumberedBasicBlock::find(const Instruction *A,
+                                          const Instruction *B) {
+  const Instruction *Inst = nullptr;
----------------
Indentation looks off here.

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


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



http://reviews.llvm.org/D11364