[PATCH] D69823: [IR] PR27065: Added tests for BasicBlock::removePredecessor.

Tue Nov 5 09:05:35 PST 2019

dendibakh marked an inline comment as done.
dendibakh added inline comments.

================
Comment at: llvm/test/Transforms/Inline/clone-function-no-assert-on-phis.ll:8-10
+; [[TMP81_I:%.*]] = getelementptr inbounds i8, i8* [[TMP81_I]], i64 1
+; It happens to work, because the code is dead and
+; verifier doesn't complain.
----------------
lebedev.ri wrote:
> dendibakh wrote:
> > lebedev.ri wrote:
> > > Can that happen for non-dead code?
> > > That is perfectly okay for dead, unreachable code.
> > 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.
> 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
> 
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.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69823/new/

https://reviews.llvm.org/D69823