chill added a comment.
TBH, I quite dislike the creeping abuse of `SubtargetFeature`s as code
generation options.
cf. Target.td:1477
//===----------------------------------------------------------------------===//
// SubtargetFeature - A characteristic of the chip set.
//
IMHO, since reserved registes are per-function, this strongly suggests
implementation as function attribute(s), rather than subtarget features (also
for the pre-existing r9).
It also opens the path towards possible future `__attribute__`.
================
Comment at: clang/include/clang/Basic/TargetInfo.h:944
+ /// using the corresponding -ffixed-RegName option.
+ virtual bool isRegisterReservedGlobally(StringRef RegName) const {
+ return true;
----------------
Parameter name can be omitted if unused; that would remove a potential warning.
================
Comment at: clang/lib/Basic/Targets/ARM.cpp:884
+ StringRef RegName, unsigned RegSize, bool &HasSizeMismatch) const {
+ if (RegName.equals("r6") || RegName.equals("r7") || RegName.equals("r8") ||
+ RegName.equals("r9") || RegName.equals("r10") || RegName.equals("r11") ||
----------------
Perhaps you can use here `RegName == "r6"` or string switch ?
================
Comment at: clang/lib/Basic/Targets/ARM.cpp:890
+ }
+ return false;
+}
----------------
`HasSizeMismatch` is not set along all possible paths.
================
Comment at: clang/lib/Basic/Targets/ARM.cpp:895
+ // The "sp" register does not have a -ffixed-sp option,
+ // so enable it unconditionally.
+ if (RegName.equals("sp"))
----------------
s/enable/reserve/ ?
================
Comment at: clang/lib/Basic/Targets/ARM.cpp:899
+
+ // enable rN (N:6-11) registers only if the corresponding
+ // +reserve-rN feature is found
----------------
Likewise ?
================
Comment at: clang/lib/Basic/Targets/ARM.cpp:901-902
+ // +reserve-rN feature is found
+ std::vector<std::string> &Features = getTargetOpts().Features;
+ std::string SearchFeature = "+reserve-" + RegName.str();
+ for (std::string &Feature : Features) {
----------------
These variables can be `const`.
================
Comment at: clang/lib/Basic/Targets/ARM.cpp:903-907
+ for (std::string &Feature : Features) {
+ if (Feature.compare(SearchFeature) == 0)
+ return true;
+ }
+ return false;
----------------
This explicit loop can be written like:
```
return llvm::any_of(getTargetOpts().Features(),
[&](auto &P) { return P == SearchFeature; });
```
================
Comment at: llvm/lib/Target/ARM/ARMSubtarget.h:236
+ // ReservedRRegisters[i] - R#i is not available as a general purpose
register.
+ BitVector ReservedRRegisters;
----------------
The usual designation for these registers is "GPR". Suggestion either
`ReservedGPRegisters` or just `ReservedRegisters`,
here and elsewhere.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D68862/new/
https://reviews.llvm.org/D68862
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
