[PATCH] D27592: Reimplement depedency tracking in the ImplicitNullChecks pass

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy added inline comments.


================
Comment at: lib/CodeGen/ImplicitNullChecks.cpp:203
 
-  // There may be readonly calls that we can handle in theory, but for
-  // now we don't bother since we don't handle callee clobbered
-  // registers.
-  if (MI->isCall() || MI->mayStore() || MI->hasUnmodeledSideEffects()) {
-    hasSeenClobber = true;
-    return;
-  }
+  for (auto I = Block.begin(), E = Block.end(); I != E; ++I) {
+    if (canReorder(*I, MI))
----------------
anna wrote:
> Nit: range based for loop here, unless some reason not to have it?
> ```
> for (auto *I : Block)
> ```
> 
I need the iterator because I want to assign to `Dep`.


================
Comment at: lib/CodeGen/ImplicitNullChecks.cpp:374
 
-  HazardDetector HD(*TRI, *AA);
+  SmallVector<MachineInstr *, 8> InstsSeenSoFar;
 
----------------
anna wrote:
> Just curious if there's any reason for choosing 8? I think the usual number for any sort of llvm related threshold is 6?
No reason -- 8 just seemed "more than enough".  I can change it to 6 if feel strongly about it.


https://reviews.llvm.org/D27592