yroux marked 5 inline comments as done. yroux added a comment. Thanks for the review Sam,
I'll update the patch with more tests (for CPSR/LR it was part of D76068 <https://reviews.llvm.org/D76068> but you are right it should be done here) and take your comments into account. In D76066#1921331 <https://reviews.llvm.org/D76066#1921331>, @samparker wrote: > Hi, > > I like reading your code! But I'm concerned around the lack of tests here, > specifically: > > - CPSR liveness. > - LR liveness. > - All things PIC related. > - Linkage legality. > - A negative test for Thumb-1. > - Inline asm generally concerns me too. ================ Comment at: llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp:5653 + + // Check if each of the unsafe registers are available... + bool R12AvailableInBlock = LRU.available(ARM::R12); ---------------- samparker wrote: > When you say 'available', do you mean 'dead'? I'm struggling to follow the > logic here... Yes that's it, 'available' in llvm::LiveRegUnit means no part of the register is live so it's 'dead'. The logic is to identify the unsafe cases where R12 or CPSR are live across the MBB without being defined in that MBB ================ Comment at: llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp:5663 + // Now, add the live outs to the set. + LRU.addLiveOuts(MBB); + ---------------- samparker wrote: > Do you need to clear LRU first? hmm, I don't think it is necessary to clear it, given that if the register was in the set the XXavailableInBlock is false so it doesn't matter if it is among liveouts or not the candidate will not have the flag UnsafeRegsDead and thus will not be outlined, if it wasn't and is part of the liveouts, we will reture false and the generic part of the MachineOutliner will not map the MBB and we will not outliune it. ================ Comment at: llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp:5705 + unsigned Opc = MI.getOpcode(); + if (Opc == ARM::t2IT || Opc == ARM::tPICADD || Opc == ARM::PICADD || + Opc == ARM::PICSTR || Opc == ARM::PICSTRB || Opc == ARM::PICSTRH || ---------------- samparker wrote: > I'm surprised to see IT here? Yes, sorry I need to update the comment, this is a conservative way of handling IT blocks, to avoid having it splitted between an outlined region and the call sites ================ Comment at: llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp:5761 + + // Don't touch the link register + if (MI.readsRegister(ARM::LR, &getRegisterInfo()) || ---------------- samparker wrote: > Isn't this already handled in the loop above? I prefer this notation though. Yes you are right, I'm cleaning this part ================ Comment at: llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp:5781 + if (Subtarget.isThumb()) + if (Call->getOperand(2).isReg()) + BuildMI(MBB, MBB.end(), DebugLoc(), get(ARM::tTAILJMPr)) ---------------- samparker wrote: > Looks like this MI building could be refactored. yes, I'm refactoring it Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76066/new/ https://reviews.llvm.org/D76066 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
