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

[Daniel Berlin]

dberlin added a comment.

I know you didn't build the original code that is being moved out here, and I really appreciate you taking the time to work on this :)

However, looking at it, while it seems okay for an internal implementation detail, moving it out into it's own class makes me think it needs a little work on the interface/implementation.
Hope that is okay.


================
Comment at: include/llvm/Analysis/AliasAnalysis.h:479
@@ -476,2 +478,3 @@
   /// callCapturesBefore - A convenience wrapper.
   ModRefResult callCapturesBefore(const Instruction *I, const Value *P,
+                                  uint64_t Size, DominatorTree *DT,
----------------
While you are here, can you please document the function and its arguments properly?

================
Comment at: include/llvm/Analysis/NumberedBasicBlock.h:34
@@ +33,3 @@
+
+class NumberedBasicBlock {
+private:
----------------
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.)

================
Comment at: include/llvm/Analysis/NumberedBasicBlock.h:38
@@ +37,3 @@
+  BasicBlock::const_iterator LastInstFound;
+  unsigned LastInstPos;
+  const BasicBlock *BB;
----------------
Can you add comments describing what these represent?

================
Comment at: include/llvm/Analysis/NumberedBasicBlock.h:43
@@ +42,3 @@
+  /// instruction while walking 'BB'.
+  const Instruction *find(const Instruction *A, const Instruction *B);
+
----------------
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 :)


================
Comment at: lib/Analysis/AliasAnalysis.cpp:348
@@ -347,3 +347,3 @@
 // with a smarter AA in place, this test is just wasting compile time.
-AliasAnalysis::ModRefResult AliasAnalysis::callCapturesBefore(
-    const Instruction *I, const MemoryLocation &MemLoc, DominatorTree *DT) {
+AliasAnalysis::ModRefResult
+AliasAnalysis::callCapturesBefore(const Instruction *I,
----------------
While you are here, can you please document the function and its arguments properly?

================
Comment at: lib/Analysis/CaptureTracking.cpp:196
@@ -250,1 +195,3 @@
 
+  if (!NBB) {
+    NumberedBasicBlock NewNBB(I->getParent());
----------------
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?

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


http://reviews.llvm.org/D11364