Page MenuHomePhabricator

Bug fix for shrink-wrap
AbandonedPublic

Authored by weimingz on Dec 14 2015, 11:55 AM.

Details

Reviewers
qcolombet
Summary

This patch fixes a case that not handled properly.
See bug report: https://llvm.org/bugs/show_bug.cgi?id=25824

Diff Detail

Repository
rL LLVM

Event Timeline

weimingz updated this revision to Diff 42747.Dec 14 2015, 11:55 AM
weimingz retitled this revision from to Bug fix for shrink-wrap.
weimingz updated this object.
weimingz set the repository for this revision to rL LLVM.
weimingz added a reviewer: qcolombet.
weimingz added a subscriber: llvm-commits.
qcolombet requested changes to this revision.Dec 14 2015, 11:58 AM
qcolombet edited edge metadata.

Hi Weiming,

Please comment the new code and add a test case.

Cheers,
-Quentin

This revision now requires changes to proceed.Dec 14 2015, 11:58 AM

This patch is more of a workaround to fix a bug. It seems to me the conditions of the safe point is not strong enough.
Since it's not SSA, the dominance relationship can't guarantee that the save point is before the def.

Hi Weiming,

Please comment the new code and add a test case.

Cheers,
-Quentin

Sure. I'm currently reducing the test case. It's exposed by a LTO compilation, so the original function is large. :D

Hi Weiming,

Please comment the new code and add a test case.

Cheers,
-Quentin

Hi Quentin,

I updated bugzilla with a test reduced from a 150,000 lines of IR. :D

I'm still trying to write a even simpler one with inline asm so we can check into unit test.

Hi Weiming,

Could you try if the attached patch fixes the problem?
If it does, that should be a much safer workaround.

Thanks,
-Quentin

Hi Quentin,

Yes, but I need to change one line because otherwise, it will crash with other inputs.

else ==> else if (MLI->getLoopFor(Restore)) {

// If the loop does not exit, ....
SmallVector<...> ExitBlocks;

MLI->getLoopFor(Restore)->getExitingBlocks(ExitBlocks); ==> need to check if Restore is in loop
}

Hi Weiming,

Could you try if the attached patch fixes the problem?
If it does, that should be a much safer workaround.

Thanks,
-Quentin

Thanks!

I’ll clean-up the patch and push with your additional fix.

Q.

Hi Weiming,

You can abandon this diff, the problem was fixed by r255613.

Thanks for reporting the problem!

Cheers,
-Quentin

weimingz abandoned this revision.Dec 25 2015, 5:43 PM