[PATCH] D29119: [ImplicitNullCheck] NFC isSuitableMemoryOp cleanup

[Serguei Katkov via Phabricator via llvm-commits]

skatkov created this revision.

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 next instruction will not succeed this check
as well. This results in redundant further operations.

Moreover canHoistLoadInst invoking computeDependence does the
similar check (no def in instructions before overlaps with this
instruction uses - including base register). So it makes sense
to avoid duplication of checks and rely on check in computeDependence.

If we found a candidate and it cannot be hoisted then stop processing
this basic block.


https://reviews.llvm.org/D29119

Files:
  lib/CodeGen/ImplicitNullChecks.cpp


Index: lib/CodeGen/ImplicitNullChecks.cpp
===================================================================
--- lib/CodeGen/ImplicitNullChecks.cpp
+++ lib/CodeGen/ImplicitNullChecks.cpp
@@ -297,15 +297,6 @@
   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;
 }
 
@@ -357,10 +348,9 @@
       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 =
@@ -481,9 +471,12 @@
       return false;
 
     MachineInstr *Dependence;
-    if (isSuitableMemoryOp(MI, PointerReg, InstsSeenSoFar) &&
-        canHoistLoadInst(&MI, PointerReg, InstsSeenSoFar, NullSucc,
-                         Dependence)) {
+    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;


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D29119.85714.patch
Type: text/x-patch
Size: 1952 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170125/ecaec64f/attachment.bin>