[PATCH] D69868: Allow "callbr" to return non-void values

Wed Feb 19 14:55:33 PST 2020

void added a comment.

In D69868#1883687 <https://reviews.llvm.org/D69868#1883687>, @jyknight wrote:

> Ugh, it's actually been that long, hasn't it...I'm really sorry about that. :(

No worries. Thanks for getting back to us!

> I've been actively spending time to look at this over the last couple weeks. I haven't been able to convince myself that the weird-successors and having allocatable registers across BBs here is not going to cause codegen issues after optimization passes run on it. Unfortunately, despite spending time looking into it, I also wasn't able to convince myself that this *IS* broken. Maybe someone else can chime in here and either assuage or confirm my worries?

I've been concerned about the register live-ins too (I'm less concerned about the successors issue). Is there documentation on the original decision to disallow physical register live-ins for MBBs before register allocation? We could then check to see if we're violating the original reasoning.

> I also see some other minor issues, which I need to write up, but I'd been blocking writing up a comment for that behind the larger question.
> 
> Anyways, while looking at this I started thinking it might actually be better to actually have INLINEASM_BR (both with or without outputs) _not_ be a terminator MachineInstr. (remaining a terminator in IR form, however). This would then be similar to how "invoke" works -- at the MachineInstr level, the call is not a terminator, even though it can jump out to the EH successors. And, the return-value handling remains in the same basic block as the call. I tried making that change, but doing it correctly has other impacts -- much of the code which currently special-cases isEHPad() needs to be updated (Which does mean I now feel like I have a good handle on the problems the _previous_ version of this code, which didn't split the block, had.). MachineBasicBlock::updateTerminator is a good example of the problematic cases -- it trawls the successor list to look for the "correct" successor, filtering out isEHPad successors. But unlike EH Pads, indirect targets of a INLINEASM_BR are not used only for that purpose, so can't be so quickly distinguished in the successors list.

I had a very similar idea. There are some people *cough*Chris*cough* who insist that `INLINEASM_BR` makes sense as a terminator. I don't deny that it's very tempting to make it one, but because of this instruction's behavior it doesn't *act* like a normal terminator (explicitly branching to some destination).

As for `isEHPad`, do you think it would make sense to have a generic `isIndirectTarget` predicate, which we could then use everywhere and it would automagically filter out blocks that aren't interesting?

> So, I started updating some of that code to not have that assumption. While doing so, I noticed that fixing this would also allow analyzeBranch to ignore the inlineasm branch instructions -- and remain correct -- which actually would allow llvm to do better block placement of the inlineasm_br blocks, because it enables it to move the normal successor jump/fallthrough. So, I think that would actually be a good idea to make that change.

Now that I know I'm not crazy for wanting to make INLINEASM_BR a non-terminator (I'm assuming you're not crazy at least :-), I'll give it a go. Feel free to send me patches if you have them.

> But whether it'd be _necessary_ to make such a change (or other changes) for this patch, or just a nice thing to fix, I'm still just unsure about.

Given your concerns over the patch in its current form, let's at least try the non-terminator path. If we can make it work, then your concerns will be assuaged (as would mine).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69868