This is an archive of the discontinued LLVM Phabricator instance.

[ImplicitNullCheck] Extend canReorder scope
ClosedPublic

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

Details

Summary

This change allows a re-order of two intructions if their uses
are overlapped.

Diff Detail

Event Timeline

skatkov created this revision.Jan 25 2017, 1:54 AM

This change has been done on top of https://reviews.llvm.org/D29119 in my workspace, so in diff you can see code from that change. While the changes are not overlapped sorry for inconvinience.

sanjoy requested changes to this revision.Jan 25 2017, 5:29 PM

Hi Serguei,

The code itself LGTM.

However:

Can you please try to write MIR test cases for this? That's typically
a more straightforward way to unit test codegen passes.
implicit-null-check.ll was added before the MIR infrastructure was
added to LLVM. You can get started with MIR at
http://llvm.org/docs/MIRLangRef.html, and may use
https://reviews.llvm.org/rL293126 as a template to write your own
test.

Secondly, phabricator tells me that you've changed the permissions on
the .ll files; please make sure you've fixed them before checkin.

This revision now requires changes to proceed.Jan 25 2017, 5:29 PM
skatkov updated this revision to Diff 85942.Jan 26 2017, 11:24 AM
skatkov edited edge metadata.

Sanjoy, please take a look.

sanjoy accepted this revision.Jan 26 2017, 2:50 PM

lgtm, but I believe you'll also have to edit the test case I checked in in https://reviews.llvm.org/rL293126

Let me know if you want me to check this in for you. Alternatively, you can ask someone from our Saint Petersburg office.

This revision is now accepted and ready to land.Jan 26 2017, 2:50 PM
This comment was removed by skatkov.

I will modify test and upload new version.

skatkov updated this revision to Diff 86011.Jan 26 2017, 8:52 PM

The test has been updated. It becomes positive now due to extension of the scope.

One more place should be fixed I guess.

lib/CodeGen/ImplicitNullChecks.cpp
370–371

I think this should be fixed as well as check only for defs. Will try to write a test today.

skatkov updated this revision to Diff 86249.Jan 30 2017, 12:03 AM

Asssert fixed. Also one test is added. The test passes with assert fix and fails witout it.

skatkov updated this revision to Diff 86617.Feb 1 2017, 4:08 AM
skatkov marked an inline comment as done.

Just a rebase after isSuitable... patch is landed.

This revision was automatically updated to reflect the committed changes.