This is an archive of the discontinued LLVM Phabricator instance.

[ImplicitNullCheck] NFC isSuitableMemoryOp cleanup
ClosedPublic

Authored by skatkov on Jan 25 2017, 1:50 AM.

Details

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.

Diff Detail

Event Timeline

skatkov created this revision.Jan 25 2017, 1:50 AM
sanjoy requested changes to this revision.Jan 25 2017, 5:25 PM

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.

This revision now requires changes to proceed.Jan 25 2017, 5:25 PM
skatkov updated this revision to Diff 86022.Jan 27 2017, 12:01 AM
skatkov edited edge metadata.
skatkov edited the summary of this revision. (Show Details)

Please take a look.

sanjoy accepted this revision.Jan 29 2017, 10:21 PM

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.

This revision is now accepted and ready to land.Jan 29 2017, 10:21 PM

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

lib/CodeGen/ImplicitNullChecks.cpp
476

To me it is more readable.

skatkov updated this revision to Diff 86258.Jan 30 2017, 2:40 AM
skatkov marked 7 inline comments as done.
This revision was automatically updated to reflect the committed changes.