[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 ↗

(On Diff #86022)

I'd call this enum SuitabilityResult instead.

162 ↗

(On Diff #86022)

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

168 ↗

(On Diff #86022)

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

308 ↗

(On Diff #86022)

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

490 ↗

(On Diff #86022)

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 = ..?

493 ↗

(On Diff #86022)

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

493 ↗

(On Diff #86022)

To me it is more readable.

llvm/trunk/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
+  enum SuitabilityResult { SR_Suitable, SR_Unsuitable, SR_Impossible };
+
+  /// Return SR_Suitable if \p MI a memory operation that can be used to
+  /// implicitly null check the value in \p PointerReg, SR_Unsuitable if
+  /// \p MI cannot be used to null check and SR_Impossible if there is
+  /// no sense to continue lookup due to any other instruction will not be able
+  /// to be used. \p PrevInsts is the set of instruction seen since
   /// the explicit null check on \p PointerReg.
-  bool isSuitableMemoryOp(MachineInstr &MI, unsigned PointerReg,
+  SuitabilityResult 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,
                         ArrayRef<MachineInstr *> InstsSeenSoFar,
                         MachineBasicBlock *NullSucc, MachineInstr *&Dependence);
 
static bool AnyAliasLiveIn(const TargetRegisterInfo *TRI,
                            MachineBasicBlock *MBB, unsigned Reg) {
   for (MCRegAliasIterator AR(Reg, TRI, /*IncludeSelf*/ true); AR.isValid();
        ++AR)
     if (MBB->isLiveIn(*AR))
       return true;
   return false;
 }
 
-bool ImplicitNullChecks::isSuitableMemoryOp(
-    MachineInstr &MI, unsigned PointerReg, ArrayRef<MachineInstr *> PrevInsts) {
+ImplicitNullChecks::SuitabilityResult
+ImplicitNullChecks::isSuitableMemoryOp(MachineInstr &MI, unsigned PointerReg,
+                                       ArrayRef<MachineInstr *> PrevInsts) {
   int64_t Offset;
   unsigned BaseReg;
 
   if (!TII->getMemOpBaseRegImmOfs(MI, BaseReg, Offset, TRI) ||
       BaseReg != PointerReg)
-    return false;
+    return SR_Unsuitable;
 
   // 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;
+    return SR_Unsuitable;
 
   // 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.
+  // If PointerReg has been redefined before then there is no sense to continue
+  // lookup due to this condition will fail for any further instruction.
   for (auto *PrevMI : PrevInsts)
     for (auto &PrevMO : PrevMI->operands())
       if (PrevMO.isReg() && PrevMO.getReg() &&
           TRI->regsOverlap(PrevMO.getReg(), PointerReg))
-        return false;
+        return SR_Impossible;
 
-  return true;
+  return SR_Suitable;
 }
 
 bool ImplicitNullChecks::canHoistLoadInst(
     MachineInstr *FaultingMI, unsigned PointerReg,
     ArrayRef<MachineInstr *> InstsSeenSoFar, MachineBasicBlock *NullSucc,
     MachineInstr *&Dependence) {
   auto DepResult = computeDependence(FaultingMI, InstsSeenSoFar);
   if (!DepResult.CanReorder)
bool ImplicitNullChecks::analyzeBlockForNullChecks(
 
   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,
-                         Dependence)) {
+    SuitabilityResult SR = isSuitableMemoryOp(MI, PointerReg, InstsSeenSoFar);
+    if (SR == SR_Impossible)
+      return false;
+    if (SR == SR_Suitable && canHoistLoadInst(&MI, PointerReg, InstsSeenSoFar,
+                                              NullSucc, Dependence)) {
       NullCheckList.emplace_back(&MI, MBP.ConditionDef, &MBB, NotNullSucc,
                                  NullSucc, Dependence);
       return true;
     }
 
     InstsSeenSoFar.push_back(&MI);
   }