[PATCH] D78341: Change callbr to only define its output SSA variable on the normal path, not the indirect targets.

Fri Apr 17 12:58:46 PDT 2020

nickdesaulniers added a comment.

In D78341#1989369 <https://reviews.llvm.org/D78341#1989369>, @rnk wrote:

> I think we should make this change. I have this idea that we can eliminate catchswitch by using callbr instead of invoke for Windows EH. Catchswitch effectively allows us to multiplex unwind edges from invoke and cleanupret. However, catchswitch blocks have no valid insertion point for non-phi instructions, and this causes a long tail of bugs where optimizers (somewhat reasonably) assume they can find an insertion point after any phi. Using callbr would help us make the CFG more accurate, and if we did that, we would want the return value to only be available on the normal edge.

I would be happier if there were more users of `callbr`, which would help us find more bugs and edge cases.  That would ultimately make it more resilient, and help spread the responsibility for it.

> If we do ever add support for asm blobs that have outputs along non-default edges, we can add some new IR construct for that (burn that bridge when we get there, as they say). `asmbrpad`, anyone?

Let's cross that bridge when we get there.  I don't think we need to design for something that no one has asked for yet.

In D78341#1989370 <https://reviews.llvm.org/D78341#1989370>, @rnk wrote:

> Although, I suppose you should add a verifier test that shows that we reject inputs where a use on the non-default edge is not dominated by a def.

Some tests would be good.

In D78341#1989404 <https://reviews.llvm.org/D78341#1989404>, @efriedma wrote:

> Does this do something reasonable if one of the indirect destinations of the callbr is also the normal destination?

I'm more concerned now about what we should do in the front end for these cases.  Indeed https://bugs.llvm.org/show_bug.cgi?id=45565#c5 came from the Linux kernel, and with this change patched in, I now see this compiler crash for kernel/futex.c (previously, we'd only fail an assertion, unknown what bugs awaited in the generated code).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78341