samparker added a comment. 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); ---------------- When you say 'available', do you mean 'dead'? I'm struggling to follow the logic here... ================ Comment at: llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp:5663 + // Now, add the live outs to the set. + LRU.addLiveOuts(MBB); + ---------------- Do you need to clear LRU first? ================ 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 || ---------------- I'm surprised to see IT here? ================ Comment at: llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp:5761 + + // Don't touch the link register + if (MI.readsRegister(ARM::LR, &getRegisterInfo()) || ---------------- Isn't this already handled in the loop above? I prefer this notation though. ================ 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)) ---------------- Looks like this MI building could be refactored. 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
