[PATCH] D67185: [RISCV] Add support for -ffixed-xX flags

[Luís Marques via Phabricator via cfe-commits]

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