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

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added a comment.

Bit of a drive-by comment, but I can't say I am big fan of all the string matching on the register names. Not sure if this is a fair comment, because I haven't looked closely at it yet, but could we use more the `ARM::R[0-9]` values more? Perhaps that's difficult from the Clang parts?



================
Comment at: clang/lib/Basic/Targets/ARM.cpp:902
+  std::vector<std::string> &Features = getTargetOpts().Features;
+  std::string SearchFeature = "+reserve-" + RegName.str();
+  for (std::string &Feature : Features) {
----------------
I was pointed at something similar myself recently, but if I am not mistaken then I think this is a use-after-free:

   "+reserve-" + RegName.str()

this will allocate a temp `std::string` that `SearchFeature` points to, which then gets released, and `SearchFeature` is still pointing at it.


================
Comment at: llvm/test/CodeGen/ARM/reg-alloc-with-fixed-reg-r6-modified.ll:15
+;     r6 = 10;
+;     unsigned int result = i + j + k + l +m + n + o + p;
+; }
----------------
nit: `+m` -> ` + m`


================
Comment at: llvm/test/CodeGen/ARM/reg-alloc-with-fixed-reg-r6.ll:13
+; {
+;     unsigned int result = i + j + k + l +m + n + o + p;
+; }
----------------
same nit


================
Comment at: llvm/test/CodeGen/ARM/reg-alloc-wout-fixed-regs.ll:3
+;
+; Equivalent C source code
+; void bar(unsigned int i,
----------------
As all these tests (this file and the ones above) are the same, the "equivalent C source code" is the same, perhaps move all these cases into 1 file.


================
Comment at: llvm/test/CodeGen/ARM/reg-alloc-wout-fixed-regs.ll:13
+; {
+;     unsigned int result = i + j + k + l +m + n + o + p;
+; }
----------------
same nit here


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68862/new/

https://reviews.llvm.org/D68862