Page MenuHomePhabricator

Change callbr to only define its output SSA variable on the normal path, not the indirect targets.
ClosedPublic

Authored by jyknight on Apr 16 2020, 7:20 PM.

Diff Detail

Event Timeline

jyknight created this revision.Apr 16 2020, 7:20 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 16 2020, 7:20 PM
rnk added a comment.Apr 17 2020, 11:12 AM

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.

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?

rnk accepted this revision.Apr 17 2020, 11:15 AM

There was discussion on the bugzilla bug, so please wait to get some agreement before landing this. Please don't wait for more feedback from me, I'm marking it accepted to indicate I'm happy with it.

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.

This revision is now accepted and ready to land.Apr 17 2020, 11:15 AM

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

In 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, @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.

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).

I'm more concerned now about what we should do in the front end for these cases

In the frontend, it should be impossible for the normal destination to be the same as an indirect destination. Even if there's a label immediately after the asm goto, we should create an extra basic block between the asm goto and the label to emit whatever stores etc. are necessary.

I'm more concerned now about what we should do in the front end for these cases

In the frontend, it should be impossible for the normal destination to be the same as an indirect destination. Even if there's a label immediately after the asm goto, we should create an extra basic block between the asm goto and the label to emit whatever stores etc. are necessary.

Ah, sorry, my use of these cases was ambiguous. I was not referring to the case you were describing of the indirect target and fallthrough being the same BasicBlock at the IR level.

I was asking about the C code from the Linux kernel that now crashes LLVM (which maybe is better than a failed assert) with this patch applied:

int futex_lock_pi_atomic(void) {
  int d;
  asm goto("" : "=r"(d) : : : e);
e:
  return d;
}

Which indeed does have a basic block inserted by the IR, and a PHI thanks to James' patch, but produces crashes when compiling the Linux kernel.

jyknight updated this revision to Diff 258438.Apr 17 2020, 3:13 PM

Fix crash reported by Nick, InstructionSimplify was bypassing the
Dominators code in some cases, and doing its own thing.

Thanks! My test case from the kernel now builds with Diff 258438. Tests in tree pass, but with some additional test cases added in this CL, I'd be very happy to approve. Thanks @jyknight!

The only other thing of note: this patch changes the other error I'm tracking for "asm goto with outputs." https://reviews.llvm.org/D77849. I have assertions I'm working on in https://reviews.llvm.org/D78166 and https://reviews.llvm.org/D78234. With this change applied though, I instead hit the assertion:

clang-11: ../lib/CodeGen/InlineSpiller.cpp:1194: bool (anonymous namespace)::HoistSpillHelper::isSpillCandBB(llvm::LiveInterval &, llvm::VNInfo &, llvm::MachineBasicBlock &, unsigned int &): Assertion `OrigLI.getVNInfoAt(Idx) == &OrigVNI && "Unexpected VNI"' failed.

I suspect it's the same issue; but I could creduce the input (same file and pass, but different function). Either way, I'd rather take this patch, and follow up on that. Just making a note of a bunch of related CLs.

llvm/lib/Analysis/InstructionSimplify.cpp
225
../lib/Analysis/InstructionSimplify.cpp:225:35: error: unknown type name 'CallbrInst'; did you mean 'CallBrInst'?
      !isa<InvokeInst>(I) && !isa<CallbrInst>(I))
                                  ^~~~~~~~~~
                                  CallBrInst
../include/llvm/IR/Instruction.def:137:40: note: 'CallBrInst' declared here
HANDLE_TERM_INST  (11, CallBr        , CallBrInst) // A call-site terminator
                                       ^

There's one part of this that's a little subtle: if an indirect destination is the same as the normal destination, the value is available at that destination even if the indirect edge is taken.

That's not necessarily a problem at the IR level, but we might need to special-case it in isel.

There's one part of this that's a little subtle: if an indirect destination is the same as the normal destination, the value is available at that destination even if the indirect edge is taken.

That's not necessarily a problem at the IR level, but we might need to special-case it in isel.

That's not actually possible: it results in an error "Duplicate callbr destination!".

jyknight updated this revision to Diff 259742.Apr 23 2020, 3:38 PM
jyknight marked an inline comment as done.

Updated with some tests. Committing this shortly.

This revision was automatically updated to reflect the committed changes.