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

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy planned changes to this revision.
sanjoy added a comment.

Replied to some comments inline, will upload new revision soon.



================
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.
+  ///
----------------
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.


================
Comment at: lib/CodeGen/ImplicitNullChecks.cpp:195
+ImplicitNullChecks::computeDependence(MachineInstr *MI,
+                                      ArrayRef<MachineInstr *> Block) {
+  assert(llvm::all_of(Block, canHandle) && "Check this first!");
----------------
anna wrote:
> Nit: const with pass by reference. 
Are you talking about `ArrayRef`?  `ArrayRef` are lightweight immutable objects that are generally supposed to be passed by value.


================
Comment at: lib/CodeGen/ImplicitNullChecks.cpp:217
 
-bool HazardDetector::isSafeToHoist(MachineInstr *MI,
-                                   MachineInstr *&Dependency) {
-  assert(!isClobbered() && "isSafeToHoist cannot do anything useful!");
-  Dependency = nullptr;
-
-  // Right now we don't want to worry about LLVM's memory model.  This can be
-  // made more precise later.
-  for (auto *MMO : MI->memoperands())
-    if (!MMO->isUnordered())
-      return false;
-
-  for (auto &MO : MI->operands()) {
-    if (MO.isReg() && MO.getReg()) {
-      for (auto &RegDef : RegDefs) {
-        unsigned Reg = RegDef.first;
-        MachineInstr *MI = RegDef.second;
-        if (!TRI.regsOverlap(Reg, MO.getReg()))
-          continue;
-
-        // We found a write-after-write or read-after-write, see if the
-        // instruction causing this dependency can be hoisted too.
-
-        if (MI == getUnknownMI())
-          // We don't have precise dependency information.
-          return false;
+bool ImplicitNullChecks::canReorder(MachineInstr *A, MachineInstr *B) {
+  for (auto MOA : A->operands()) {
----------------
anna wrote:
>  Could you please clarify the type of `MachineInstr` we consider in `canReorder` using asserts? IIUC, only one of A or B can be a memory instruction (this is verified by caller of `canReorder`). 
> 
> If both are memory instructions, this method can incorrectly return true. 
Good point, I'll either add an explicit check or add an assert.


================
Comment at: lib/CodeGen/ImplicitNullChecks.cpp:435
+      //
+      // This restriction does not apply to the faulting load inst because TODO.
+      if (AnyAliasLiveIn(TRI, NullSucc, DependenceMO.getReg()))
----------------
anna wrote:
> What does the comment mean? Couldn't find any TODO.
Ugh, I meant to fill in that comment before uploading the patch (the TODO was really "TODO - complete the comment").  I'll fix this in the next iteration.


================
Comment at: lib/CodeGen/ImplicitNullChecks.cpp:441
+      // won't get the memory operation on the address we want.  FIXME
+      assert(!TRI->regsOverlap(DependenceMO.getReg(), PointerReg) &&
+             "Should have been checked before!");
----------------
anna wrote:
> this assert is also valid because of the check done at the end of `IsSuitableMemoryOp`: line 386, right?
Yes.


https://reviews.llvm.org/D27592