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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
