void added a comment. In D69868#1836777 <https://reviews.llvm.org/D69868#1836777>, @jyknight wrote:
> The idea of moving the copies to a new MachineBasicBlock seems a reasonable > solution. That said, it does mean there will be allocatable physical > registers which are live-in to the following block, which is generally not > allowed. As far as I can tell, I _think_ that should be fine in this > particular circumstance, but I'm a little uneasy that I might be missing some > reason why it'll be incorrect. There are some passes (e.g. machine CSE; MachineCSE.cpp:692) that handle physical live-in registers before register allocation. Do those passes run after a pass which handles the physical live-in registers? (By "handle" I mean analyzes and/or modifies the machine IR so that physical live-ins are "okay".) > I'm more uncomfortable with the scanning/splitting after-the-fact in > ScheduleDAGSDNodes.cpp, and then the successor lists subsequently being > incorrect. However, I wasn't immediately able to say what I'd suggest doing > instead, so I spent some time yesterday poking around with this patch to see > if I could find something that seemed better. I now believe it will be > cleaner to have the inline-asm tell SelectionDAGISel::FinishBasicBlock to > just emit the copies in another block, which we do in some other situations, > already. But, I'm still poking at that to see how it'll end up -- I can't say > I'm sure that's the right answer at the moment. I looked at FinishBasicBlock at one point. I put it in EmitSchedule because I wanted to allow the basic block to go through any post processing that may occur. I can place it in FinishBasicBlock if you think it fits in there better. As for the successor list, I justify it this way: The default behavior is exactly the same as executing the inline asm and continuing directly out as if it's straight line code. If INLINEASM_BR wasn't a terminator, we would add the COPYs directly after it and before the JMP. The only issue is whether the successors being on the copy block instead of the block containing the INLINEASM_BR would cause something to go wrong. Like you I was concerned about this. But I think that since the behavior is the same as if the two blocks were a single block (the assembly code isn't changed, etc.), and the fact that the successors being on the INLINEASM_BR block shouldn't affect any machine passes (i.e. no analysis or transformation should care, because the IR can't model potential branches by the asm), it should be okay. If you wish I could add a part to the machine instruction verifier to ensure that assumptions we're making are enforced. 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
