[PATCH] D69865: [IR] PR27065: Part2. Fix BasicBlock::removePredecessor to not break SSA form.

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

In D69865#1737382 <https://reviews.llvm.org/D69865#1737382>, @dendibakh wrote:

> I'm not arguing that passes should not look into unreachable code.
>
> But in this exact case, BasicBlock::removePredecessor is pretty much general API, not limited to particular pass. We are not 100% sure that the broken code will be unreachable. 
>  Consider this:
>
>   define i32 @f(i1 %cmp, i8 *%call) {
>   entry:
>     br i1 %cmp, label %phi.bb, label %ret.bb
>   phi.bb:
>     %ptr.1 = phi i8* [ %call, %entry],[ %inc.ptr1, %use.bb ]
>     br label %use.bb
>   use.bb:
>     %inc.ptr1 = getelementptr inbounds i8, i8* %ptr.1, i64 1
>     br i1 %cmp, label %ret.bb, label %phi.bb
>   ret.bb:
>     ret i32 0
>   }
>
>
> After doing:
>
>   PhiBB->removePredecessor(EntryBB);
>
>
> We have:
>
>   define i32 @f(i1 %cmp, i8* %call) {
>   entry:
>     br i1 %cmp, label %phi.bb, label %ret.bb				<== edge entry->phi.bb will be removed later
>   phi.bb:
>     br label %use.bb
>   use.bb:
>     %inc.ptr1 = getelementptr inbounds i8, i8* %inc.ptr1, i64 1 		<== broken
>     br i1 %cmp, label %ret.bb, label %phi.bb
>   ret.bb:
>     ret i32 0
>   }
>


While in the middle of a code rewrite, code can be broken. I don't see a way around that.

> User can later decide to link phi.bb into some other place (outlining, for example). Can that happen? If yes, it is not unreachable code anymore.

Arguably, you can do that but you can do a lot of things that will result in problems. If you disconnect a block with a phi you are already at a point that does not make much sense. You argue that keeping the phi around as a "known" value that needs to be replaced once the block is reconnected, though it seems more like a workaround to me. Once disconnected, IR looses it's semantic, especially PHI nodes.

> To summarize my point: even if we know the code is dead and noone will look into it, I don't see benefits of allowing it to be broken. In this particular case, (I hope) it will require no additional changes, since passes do not look into unreachable code and do not care if it is valid or not.

The benefit is we don't need special cases trying to not "break" it:

- They will probably never be complete, thus do not remove the need to deal with unreachable code appropriately.
- They will probably introduce conservative regressions and workarounds, as the one you added: https://reviews.llvm.org/D69865#C1676614NL685


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69865