[PATCH] D86215: [TableGen][GlobalISel] Fix handling of zero_reg

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added inline comments.


================
Comment at: llvm/utils/TableGen/CodeGenTarget.cpp:547
 
+StringRef CodeGenTarget::getRegNamespace() const {
+  auto &RegClasses = RegBank->getRegClasses();
----------------
Just like in the header file. I'd rather put this next to getInstNamespace. This would also show more clearly that we currently fetch the namespaces in different ways, and by that we perhaps support different namespaces for regs and instructions etc. I have no idea if that is used (or useful) in reality.

I figure that  we at least wouldn't break anything as long as we follow what CodeGenRegisters is using when defining the enum with NoRegister, when fetching the getRegNamespace.


================
Comment at: llvm/utils/TableGen/CodeGenTarget.h:194
+  /// getRegNamespace - get namespace used for the registers.
+  StringRef getRegNamespace() const;
+
----------------
I'd put this next to getInstNamespace(). Or if for some reason it would be better to keep "inst" helpers and "reg" helpers separated, then I'd put it next to something related to registers such as getRegBank or getRegRegisterByName. But I can't see any logic in the current order of methods (it's even mixing public and private declarations), so that's why I'd got for keeping the namespace-related functions close to each other.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86215