luismarques accepted this revision. luismarques added a comment. This revision is now accepted and ready to land.
Overall LGTM. Caveats: - Address the issues in the inline comments; - Shouldn't the TLS lowering also complain when `-ffixed-x4` is used? - Is there a way to ensure we don't forget to check any such reserved reg uses? I'm not quite confident we haven't overlooked anything. - (Remember to check for the `-ffixed-xX` flags when implementing the callee-saved regs via libcalls (D62686 <https://reviews.llvm.org/D62686>), etc.) Apologies for the delayed review. ================ Comment at: clang/include/clang/Driver/Options.td:2224 HelpText<"Don't workaround Cortex-A53 erratum 835769 (AArch64 only)">; -foreach i = {1-7,9-15,18,20-28} in - def ffixed_x#i : Flag<["-"], "ffixed-x"#i>, Group<m_aarch64_Features_Group>, - HelpText<"Reserve the "#i#" register (AArch64 only)">; +foreach i = {1-31} in + def ffixed_x#i : Flag<["-"], "ffixed-x"#i>, Group<m_Group>, ---------------- Given the expansion of the flags here, the AArch64 driver should probably detect and reject the flags `-ffixed-x[8,16-17,19,29-31]`, to preserve the old behavior where passing those flags would be an error and to ensure that erroneous flags are not silently accepted. ================ Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:2412 + })) + F.getContext().diagnose(DiagnosticInfoUnsupported{F, "Argument register" + " required, but has been reserved."}); ---------------- clang-format indicates another formatting style here. ================ Comment at: llvm/lib/Target/RISCV/RISCVSubtarget.cpp:53 : RISCVGenSubtargetInfo(TT, CPU, FS), + UserReservedRegister(RISCV::NUM_TARGET_REGS), FrameLowering(initializeSubtargetDependencies(TT, CPU, FS, ABIName)), ---------------- This includes more than the x0 - x31 registers. If the intent is to only allow reserving the GPRs then this should be tightened. ================ Comment at: llvm/lib/Target/RISCV/RISCVSubtarget.h:98 + bool isRegisterReservedByUser(size_t i) const { + return UserReservedRegister[i]; + } ---------------- Consider adding a bounds checking assert. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67185/new/ https://reviews.llvm.org/D67185 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
