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

[Anna Thomas via Phabricator via llvm-commits]

anna added inline comments.


================
Comment at: lib/CodeGen/ImplicitNullChecks.cpp:77
+  /// A data type for representing the result computed by \c
+  /// computeDependence.
+  struct DependenceResult {
----------------
Add the fact that we handle at most one dependent instruction?


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


================
Comment at: lib/CodeGen/ImplicitNullChecks.cpp:195
+ImplicitNullChecks::computeDependence(MachineInstr *MI,
+                                      ArrayRef<MachineInstr *> Block) {
+  assert(llvm::all_of(Block, canHandle) && "Check this first!");
----------------
Nit: const with pass by reference. 


================
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()) {
----------------
 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. 


================
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()))
----------------
What does the comment mean? Couldn't find any TODO.


================
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!");
----------------
this assert is also valid because of the check done at the end of `IsSuitableMemoryOp`: line 386, right?


https://reviews.llvm.org/D27592