[PATCH] D27457: [globalisel] Tablegen-erate PartialMapping*/ValueMapping*/BankIDToCopyMapIdx.

[Daniel Sanders via Phabricator via llvm-commits]

dsanders added inline comments.


================
Comment at: utils/TableGen/RegisterBankEmitter.cpp:252
 
 void RegisterBankEmitter::emitRegisterBankInfoImplementation(
     raw_ostream &OS, StringRef TargetName,
----------------
rovka wrote:
> This function is growing a bit long, could you split it up a bit?
Ok.


================
Comment at: utils/TableGen/RegisterBankEmitter.cpp:266
+    for (const auto &PartialMappingSize : Bank.partial_mappings())
+      OS << "  PMI_" << Bank.getName() << PartialMappingSize << ",\n";
+
----------------
rovka wrote:
> These used to start at 1, they will start at 0 now - is this intentional?
Yes, but it might be better to leave it for a later patch. I've only got a weak argument for either value.

The argument in favour of 0 is that that's what it was before r288812. That commit changed it to 1 to demonstrate that it had caught all the places where it was either using -PMI_FirstGPR (calls FirstGPR at the time) or had forgotten to include a subtraction. Now that this has been fixed, it's getting less important as we tablegen-erate more.

The argument in favour of 1 is that there's still a couple bits that haven't been tablegen-erated yet such as the asserts in AArch64RegisterBankInfo::AArch64RegisterBankInfo().


================
Comment at: utils/TableGen/RegisterBankEmitter.cpp:270
+  for (const auto &Bank : Banks) {
+    if (Bank.partial_mappings().begin() != Bank.partial_mappings().end()) {
+      if (FirstEmittedBank == nullptr)
----------------
rovka wrote:
> Bank.hasPartialMappings()?
Thanks. I missed this one.


https://reviews.llvm.org/D27457