[ImplicitNullCheck] NFC isSuitableMemoryOp cleanup

[skatkov]

Summary

isSuitableMemoryOp method is repsonsible for verification
that instruction is a candidate to use in implicit null check.
Additionally it checks that base register is not re-defined before.
In case base has been re-defined it just returns false and lookup
is continued while any suitable instruction will not succeed this check
as well. This results in redundant further operations.

So when we found that base register has been re-defined we just
stop.

[sanjoy]

Hi Serguei,

This change is NFC today only because of the two existing bugs you
pointed out offline. That is, once we make these changes:

diff --git a/lib/CodeGen/ImplicitNullChecks.cpp b/lib/CodeGen/ImplicitNullChecks.cpp
index 0440555..36f984f 100644
--- a/lib/CodeGen/ImplicitNullChecks.cpp
+++ b/lib/CodeGen/ImplicitNullChecks.cpp
@@ -248,7 +248,7 @@ bool ImplicitNullChecks::canReorder(const MachineInstr *A,
 
       unsigned RegB = MOB.getReg();
 
-      if (TRI->regsOverlap(RegA, RegB))
+      if (TRI->regsOverlap(RegA, RegB) && (MOA.isDef() || MOB.isDef()))
         return false;
     }
   }
@@ -302,7 +302,7 @@ bool ImplicitNullChecks::isSuitableMemoryOp(
   // between the compare and the load.
   for (auto *PrevMI : PrevInsts)
     for (auto &PrevMO : PrevMI->operands())
-      if (PrevMO.isReg() && PrevMO.getReg() &&
+      if (PrevMO.isReg() && PrevMO.getReg() && PrevMO.isDef() &&
           TRI->regsOverlap(PrevMO.getReg(), PointerReg))
         return false;

this change will no longer be NFC and will regress test cases like
https://reviews.llvm.org/rL293126.

I'd say let's hold this specific refactoring till you've fixed the two
issues you found to make sure we end up with a pass that is not
needlessly pessimistic.

[skatkov]

Please take a look.

[sanjoy]

lgtm with very minor nits

I'll submit this patch on your behalf once you upload a new version with the nits fixed here on phabricator

lib/CodeGen/ImplicitNullChecks.cpp

161

I'd call this enum SuitabilityResult instead.

162

I'd call these SR_Suitable, SR_Unsuitable, and SR_Impossible (especially since SR_Continue is somewhat ambiguous).

162

This comment needs updated since we're no longer returning a boolean.

299–300

Add a short line here on why you're returning SR_Impossible.

473–474

SRResult does not expand out correctly (it sounds like you're trying to say "suitability result result").

How about the more llvm idiomatic SuitableResult SR = ..?

476

The braces around SRResult == SR_OK is unnecessary.

[skatkov]

Will appload a new version soon. I'm ok with all comments.

lib/CodeGen/ImplicitNullChecks.cpp

476

To me it is more readable.

lib/CodeGen/ImplicitNullChecks.cpp

class ImplicitNullChecks : public MachineFunctionPass {
   MachineModuleInfo *MMI = nullptr;
 
   bool analyzeBlockForNullChecks(MachineBasicBlock &MBB,
                                  SmallVectorImpl<NullCheck> &NullCheckList);
   MachineInstr *insertFaultingLoad(MachineInstr *LoadMI, MachineBasicBlock *MBB,
                                    MachineBasicBlock *HandlerMBB);
   void rewriteNullChecks(ArrayRef<NullCheck> NullCheckList);
 
   /// Is \p MI a memory operation that can be used to implicitly null check the
   /// value in \p PointerReg?  \p PrevInsts is the set of instruction seen since
   /// the explicit null check on \p PointerReg.
   bool isSuitableMemoryOp(MachineInstr &MI, unsigned PointerReg,
                           ArrayRef<MachineInstr *> PrevInsts);
 
   /// Return true if \p FaultingMI can be hoisted from after the the
   /// instructions in \p InstsSeenSoFar to before them.  Set \p Dependence to a
   /// non-null value if we also need to (and legally can) hoist a depedency.
   bool canHoistLoadInst(MachineInstr *FaultingMI, unsigned PointerReg,
bool ImplicitNullChecks::isSuitableMemoryOp(
   if (!TII->getMemOpBaseRegImmOfs(MI, BaseReg, Offset, TRI) ||
       BaseReg != PointerReg)
     return false;
 
   // We want the load to be issued at a sane offset from PointerReg, so that
   // if PointerReg is null then the load reliably page faults.
   if (!(MI.mayLoad() && !MI.isPredicable() && Offset < PageSize))
     return false;
 
-  // Finally, we need to make sure that the load instruction actually is
-  // loading from PointerReg, and there isn't some re-definition of PointerReg
-  // between the compare and the load.
-  for (auto *PrevMI : PrevInsts)
-    for (auto &PrevMO : PrevMI->operands())
-      if (PrevMO.isReg() && PrevMO.getReg() &&
-          TRI->regsOverlap(PrevMO.getReg(), PointerReg))
-        return false;
-
   return true;
 }
 
 bool ImplicitNullChecks::canHoistLoadInst(
     MachineInstr *FaultingMI, unsigned PointerReg,
     ArrayRef<MachineInstr *> InstsSeenSoFar, MachineBasicBlock *NullSucc,
     MachineInstr *&Dependence) {
   auto DepResult = computeDependence(FaultingMI, InstsSeenSoFar);
   if (!DepResult.CanReorder)
for (auto &DependenceMO : DependenceMI->operands()) {
     // semantically execute, and affect machine state.  That is, if the load
     // was loading into %rax and it faults, the value of %rax should stay the
     // same as it would have been had the load not have executed and we'd have
     // branched to NullSucc directly.
     if (AnyAliasLiveIn(TRI, NullSucc, DependenceMO.getReg()))
       return false;
 
     // The Dependency can't be re-defining the base register -- then we won't
-    // get the memory operation on the address we want.  This is already
-    // checked in \c IsSuitableMemoryOp.
-    assert(!TRI->regsOverlap(DependenceMO.getReg(), PointerReg) &&
-           "Should have been checked before!");
+    // get the memory operation on the address we want. So bail out.
+    if (TRI->regsOverlap(DependenceMO.getReg(), PointerReg))
+      return false;
   }
 
   auto DepDepResult =
       computeDependence(DependenceMI, {InstsSeenSoFar.begin(), DependenceItr});
 
   if (!DepDepResult.CanReorder || DepDepResult.PotentialDependence)
     return false;
 
bool ImplicitNullChecks::analyzeBlockForNullChecks(
   const unsigned PointerReg = MBP.LHS.getReg();
 
   SmallVector<MachineInstr *, 8> InstsSeenSoFar;
 
   for (auto &MI : *NotNullSucc) {
     if (!canHandle(&MI) || InstsSeenSoFar.size() >= MaxInstsToConsider)
       return false;
 
     MachineInstr *Dependence;
-    if (isSuitableMemoryOp(MI, PointerReg, InstsSeenSoFar) &&
-        canHoistLoadInst(&MI, PointerReg, InstsSeenSoFar, NullSucc,
+    if (isSuitableMemoryOp(MI, PointerReg, InstsSeenSoFar)) {
+      if (!canHoistLoadInst(&MI, PointerReg, InstsSeenSoFar, NullSucc,
                            Dependence)) {
+        // If we cannot hoist this load then we will not be able to hoist any.
+        return false;
+      }
       NullCheckList.emplace_back(&MI, MBP.ConditionDef, &MBB, NotNullSucc,
                                  NullSucc, Dependence);
       return true;
     }
 
     InstsSeenSoFar.push_back(&MI);
   }