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

[Anna Thomas via Phabricator via llvm-commits]

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

LGTM with comments.



================
Comment at: lib/CodeGen/ImplicitNullChecks.cpp:97
+  /// Compute a result for the following question: can \p MI be
+  /// re-ordered from after \p Insts to before it.
+  ///
----------------
sanjoy wrote:
> anna wrote:
> > This is actually not true right? There are checks done after getting the dependent instruction, to make sure that the register in dependent instruction does not overlap with the `PointerReg`.
> That check is required not to check the validity of the hoist, but to check if the hoisted pair of instructions can be used to do an implicit null check on `PointerReg`.  That is, if we didn't care about implicit null checking, but only cared for hoisting, that check in `PointerReg` won't be necessary.
agreed.


================
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))
----------------
Nit: range based for loop here, unless some reason not to have it?
```
for (auto *I : Block)
```



================
Comment at: lib/CodeGen/ImplicitNullChecks.cpp:374
 
-  HazardDetector HD(*TRI, *AA);
+  SmallVector<MachineInstr *, 8> InstsSeenSoFar;
 
----------------
Just curious if there's any reason for choosing 8? I think the usual number for any sort of llvm related threshold is 6?


https://reviews.llvm.org/D27592