[PATCH] D82269: [TRE][NFC] Refactor Basic Block Processing

[Layton Kifer via Phabricator via llvm-commits]

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


================
Comment at: llvm/lib/Transforms/Scalar/TailRecursionElimination.cpp:774
+    // eliminateCall will attempt to remove.
+    if (!Succ->hasAddressTaken() && pred_begin(Succ) == pred_end(Succ))
+      DTU.deleteBB(Succ);
----------------
efriedma wrote:
> laytonio wrote:
> > efriedma wrote:
> > > I'm pretty sure the address-taken check here is wrong, and this doesn't interact correctly with unreachable code.  Probably we should teach eliminateCall to RAUW away dead uses instead, and then let some other code call removeUnreachableBlocks to do the cleanup properly.
> > > 
> > > I guess this isn't new code, though.
> > I don't think there is anything we could use to RAUW that would be semantic preserving, not that it matters much I guess since this block is now dead. I looked at the other uses of FoldReturnIntoUncondBranch (there are only 2) to see how other passes handle the possibility of creating a dead block. SimplifyCFG just checks the predecessors and deletes the block. CodeGenPrepare checks the predecessors and the address-taken and deletes the block. The BasicBlock destructor seems to correctly handle the case where a dead block has its address taken, and even a nice comment explaining how you could end up in that situation. Therefore, I think we are safe to remove the address-taken check and just delete the block if it has no predecessors.
> > I don't think there is anything we could use to RAUW that would be semantic preserving, not that it matters much I guess since this block is now dead.
> 
> Exactly, the value doesn't matter because it's dead.
> 
> > I looked at the other uses of FoldReturnIntoUncondBranch (there are only 2) to see how other passes handle the possibility of creating a dead block. SimplifyCFG just checks the predecessors and deletes the block. CodeGenPrepare checks the predecessors and the address-taken and deletes the block
> 
> Consider the following example, which currently crashes tailcallelim:
> 
> ```
> define i32 @foo() {
>   %z = call i32 @foo()
>   ret i32 %z
> 
>   ret i32 %z
> }
> ```
> 
> removeUnreachableBlocks avoids the problem by calling dropAllReferences() (which only works because it's erasing all the unreachable blocks at once).  CodeGenPrepare avoids the problem by accident, I think; some of the checks implicitly exclude or manipulate dead code.
I feel like I may be misunderstanding what you're saying. In your example you are passing in a function with a dead block. Where as this only applies to something like the following:

```
define i32 @foo() {
  %z = call i32 @foo()
  br label %retblock

retblock:
  ret i32 %z
}
```
Where we transform to this:
```
define i32 @foo() {
  %z = call i32 @foo()
  ret i32 %z

retblock:
  ret i32 %z
}
```

In order to eliminate the call. Is it not valid to just delete the now dead block? It seems a little bug prone to just RAUW when the replacement doesn't maintain the semantics, but that is just my opinion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82269