[PATCH] D31937: Move value type list from TargetRegisterClass to TargetRegisterInfo

[Alex Bradbury via Phabricator via llvm-commits]

asb requested changes to this revision.
asb added a comment.
This revision now requires changes to proceed.

I had a couple of minor review comments. I know the use of pointers vs references is quite inconsistent across LLVM, but I do feel it would be an improvement if `isLegalRC` took a `const TargetRegisterInfo&` (though perhaps there's a reason not to do this that I'm missing?).



================
Comment at: include/llvm/Target/TargetLowering.h:2265
   /// register class are all legal.
-  bool isLegalRC(const TargetRegisterClass *RC) const;
+  bool isLegalRC(const TargetRegisterInfo *TRI,
+                 const TargetRegisterClass &RC) const;
----------------
Given that `TRI` must be non-null in the isLegalRC implementation later on in this patch, perhaps this function should take a reference to TargetRegisterInfo?


================
Comment at: include/llvm/Target/TargetRegisterInfo.h:335
+    vt_iterator I = RC.VTs;
+    while (*I != MVT::Other) ++I;
+    return I;
----------------
Nitpick: I know this is just copied from the old `vt_end`, but having `++I` on the next line would be more usual LLVM coding style (and is what clang-format would produce).


================
Comment at: lib/CodeGen/TargetLoweringBase.cpp:1189
+                                   const TargetRegisterClass &RC) const {
+  for (auto I = TRI->valuetypes_begin(RC); *I != MVT::Other; ++I)
     if (isTypeLegal(*I))
----------------
Even though it's slightly less typing, I can't help but feel that checking for MVT::Other is peeking under the iterator abstraction unnecessarily.


Repository:
  rL LLVM

https://reviews.llvm.org/D31937