Page MenuHomePhabricator

[PHIElimination] Update the regression test for PR16508
ClosedPublic

Authored by bjope on Sep 26 2018, 7:54 AM.

Details

Summary

When PR16508 was solved (in rL185363) a regression test was
added as test/CodeGen/PowerPC/2013-07-01-PHIElimBug.ll.
I discovered that the test case no longer reproduced the
scenario from PR16508. This problem could have been amended
by adding an extra RUN line with "-O1" (or possibly "-O0"),
but instead I added a mir-reproducer

test/CodeGen/PowerPC/2013-07-01-PHIElimBug.mir

to get a reproducer that is less sensitive to changes in
earlier passes (including O-level).

While being at it I also corrected a code comment in
PHIElimination::EliminatePHINodes that has been incorrect
since the related bugfix from rL185363.

Diff Detail

Repository
rL LLVM

Event Timeline

bjope created this revision.Sep 26 2018, 7:54 AM
MatzeB accepted this revision.Sep 27 2018, 9:58 AM

LGTM, thanks! Maybe we can even drop the .ll test now?

This revision is now accepted and ready to land.Sep 27 2018, 9:58 AM
bjope added a comment.Sep 28 2018, 1:15 AM

LGTM, thanks! Maybe we can even drop the .ll test now?

Original plan was to remove the .ll test. Then I read about LiveVariables being deprecated, and since I use livevars it in the new .mir test I though that perhaps that test case will be invalid in the future.

Now when I've got the bigger picture about the history regarding LiveVariable vs LiveIntervals in this pass I have this suggestion:

  1. I'll remove the .ll test case in this patch.
  2. I'll add comments new .mir test case (above the RUN line), describing that this is a reduced reproducer based on the old .ll test case.

Given the information in the .mirtest (and the commit msg) it should be quite easy to dig up the old .ll test case again, if needed in the future, for example if livevars is removed.

I'll update the patch according to the above.

bjope updated this revision to Diff 167483.Sep 28 2018, 8:38 AM

Removed the .ll test case (and updated comments in the new .mir test) as suggested in the review comments.

bjope edited the summary of this revision. (Show Details)Sep 28 2018, 8:40 AM
This revision was automatically updated to reflect the committed changes.