[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).

168

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

308

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

490

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

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

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);
 
+  enum SuitableResult {
+    SR_OK,
+    SR_Continue,
+    SR_Impossible
+  };
+
   /// 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,
+  SuitableResult 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(
+ImplicitNullChecks::SuitableResult 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_Continue;
 
   // 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_Continue;
 
   // 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 SR_Impossible;
 
-  return true;
+  return SR_OK;
 }
 
 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) &&
+    SuitableResult SRResult = isSuitableMemoryOp(MI, PointerReg, InstsSeenSoFar);
+    if (SRResult == SR_Impossible)
+      return false;
+    if ((SRResult == SR_OK) &&
         canHoistLoadInst(&MI, PointerReg, InstsSeenSoFar, NullSucc,
                          Dependence)) {
       NullCheckList.emplace_back(&MI, MBP.ConditionDef, &MBB, NotNullSucc,
                                  NullSucc, Dependence);
       return true;
     }
 
     InstsSeenSoFar.push_back(&MI);