[PATCH] D88823: [CodeGen][TailDuplicator] Don't duplicate blocks with INLINEASM_BR

Mon Oct 5 19:58:02 PDT 2020

void added a comment.

In D88823#2313337 <https://reviews.llvm.org/D88823#2313337>, @jyknight wrote:

>> When we go to resolve this PHI node, there's no place to put the put the values that works in all situations; we can't split the critical edges and it's not valid to place the resolved instructions at the ends of the predecessor blocks.
>
> We can (at least: should be able to) handle a PHI node in the indirect targets of an INLINEASM_BR, for any value which is not the output of the INLINEASM_BR itself. We place the required register copies prior to the INLINEASM_BR, rather than after it. This is handled by findPHICopyInsertPoint -- where we did have a bug already, fixed in f7a53d82c0902147909f28a9295a9d00b4b27d38 <https://reviews.llvm.org/rGf7a53d82c0902147909f28a9295a9d00b4b27d38>.

I disagree. You'll be inserting an instruction that won't be executed on the default path before the asm goto block. While this may be okay in a majority of cases, I don't suspect that it'll be good to rely on it being always okay.

> I also don't see anything wrong in this test-case. AFAICT, the phi looks to be handled correctly here. Perhaps it's been minimized too far, and is no longer showing the actual problem?

The testcase is very simplified from the original. The testcase replicates the problematic tail duplication, but not the issue that caused the memory corruption. You can see the .ll files in the bug that's associated with it.

I've explained way too many times what this patch is doing. You and Nick seem to think that this patch is bad for some reason or that it's confusing. If so, then please present one of your own.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88823