[PATCH] D29119: [ImplicitNullCheck] NFC isSuitableMemoryOp cleanup

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy requested changes to this revision.
sanjoy added a comment.
This revision now requires changes to proceed.

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.


https://reviews.llvm.org/D29119