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

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

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

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

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.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69868