Page MenuHomePhabricator

[IR] [NFC] PR27065: Added tests for BasicBlock::removePredecessor.
Needs RevisionPublic

Authored by dendibakh on Nov 4 2019, 1:27 PM.

Details

Summary

This patch precommits tests that expose bugs in BasicBlock::removePredecessor.
In certain cases calling this function breaks SSA form.
Those bugs will be fixed in upcoming patches.

Diff Detail

Event Timeline

dendibakh created this revision.Nov 4 2019, 1:27 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 4 2019, 1:27 PM
lebedev.ri added inline comments.Nov 4 2019, 1:32 PM
llvm/test/Transforms/Inline/clone-function-no-assert-on-phis.ll
8–10

Can that happen for non-dead code?
That is perfectly okay for dead, unreachable code.

dendibakh marked an inline comment as done.Nov 4 2019, 2:58 PM
dendibakh added inline comments.
llvm/test/Transforms/Inline/clone-function-no-assert-on-phis.ll
8–10

I think it doesn't happen for non-dead code. Otherwise, verifier would tell us about that.
Regarding BasicBlock::removePredecessor behavior: this is okay to leave broken code in unreachable blocks until someone will try to link detached blocks again. And we never know what will happen after user called removePredecessor. In this sense BasicBlock::removePredecessor should not leave broken code.

lebedev.ri added inline comments.
llvm/test/Transforms/Inline/clone-function-no-assert-on-phis.ll
8–10

Please can you be more specific?
Is there more complete test case that demonstrates the issues this causes?

https://bugs.llvm.org/show_bug.cgi?id=27065
(which is no longer reproducible since that pass is gone)
reads to me as classical "pass gets overwhelmed by
the wonders of unreachable IR", which is solved by
not handling unreachable code, which also is faster:
rL372339 rL370911 rL355337 etc

dendibakh marked an inline comment as done.Nov 5 2019, 8:58 AM
dendibakh added inline comments.
llvm/test/Transforms/Inline/clone-function-no-assert-on-phis.ll
8–10

I don't have a test case for LLVM where this would cause miscompilation or compiler crash. But I ran into issues in our downstream. If you look from an API perspective, BasicBlock::removePredecessor violates its 'contract'. I mean it could leave broken code after itself. And we are not sure noone will use this code afterwards.

Moreover, implementation of BasicBlock::removePredecessor has serious issue and happens to work just by luck. I will upload the fix to showcase this.

As for the second part, yes, the repro in the LLVM bug tracker is no longer valid. You can find small examples in the unittests attached to this patch.

dendibakh retitled this revision from [IR] PR27065: Added tests for BasicBlock::removePredecessor. to [IR] [NFC] PR27065: Added tests for BasicBlock::removePredecessor..Nov 6 2019, 8:14 AM

Here are the 2 follow-up patches: D69862, D69865

lebedev.ri requested changes to this revision.Nov 10 2019, 9:33 AM

(as per disscussion in https://reviews.llvm.org/D69865)

This revision now requires changes to proceed.Nov 10 2019, 9:33 AM