[PATCH] D68862: [ARM] Allocatable Global Register Variables for ARM

[Momchil Velikov via Phabricator via llvm-commits]

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