[PATCH] D27338: [globalisel] Tablegen-erate current Register Bank Information

[Daniel Sanders via Phabricator via llvm-commits]

dsanders marked an inline comment as done.
dsanders added a comment.

In https://reviews.llvm.org/D27338#618721, @ab wrote:

> This seems reasonable, but:
>
> - I don't think the PartialMapping exclusion mechanism is necessary.  What about adding the missing mappings manually before landing this patch?  Alternatively, replace the manual code with a copy of the generated mappings (once you have those) before any tablegen changes?  That lets us split the generalization from the NFC tablegen changes without requiring hacks in-between.


It should be easy enough to add FPR's 8 and 16 mapping and CCR's 32 mapping. I'm not sure about FPR's 192 and 384 mappings though. There's definitely some code that assumes the sizes are a power of 2 (e.g. getRegBankBaseIdxOffset()) but I'm not sure how widespread that assumption is. I'd have to investigate.

> - I'm not sure it's useful to generate the asserts: I think the intent was to check 1) the correctness of the manual description, and 2) it being consistent with the eventual tablegen output. Have you considered keeping them in AArch64RegisterBankInfo?  I suppose generating them lets all targets have them. @qcolombet: what do you think?

I suppose it depends which direction you look at it. It makes sense to me to have the manual description check that the tablegen part meets its assumptions, but I also think it makes sense to have the tablegen-erated code check that it meets the assumptions of the manual code it's replacing.

For the assertions about PMI_* values, I think the majority of these will become unnecessary once the remaining pieces of AArch64GenRegisterBankInfo.def are tablegen-erated. Once that's done, I think it will all be internal to the tablegen-erated code except for the assumption that PMI_First* can be used to specify the register bank and I think we ought to use *RegBankID's for that instead.

For the assertions that call RB.covers(). I think this can be checked in either direction but it's a bit easier for tablegen to check that all the expected classes have been added since it's easy to forget a particular class when listing them manually.

For the assertions that call RB.getSize(). I think both places are equally good.

> - Also, can you extract the RegisterBank code to a shared location? (CodeGenRegBank? maybe rename it to CodeGenRegisters) ?  That'll let us reuse it from the selector emitter backend.  We can do that separately though.

That makes sense to me. Is it ok if I do that as another patch at the end of my current patch-series? I could insert one before this one too but adding it to the end should save me some rebasing.



================
Comment at: utils/TableGen/RegisterBankEmitter.cpp:343
+void RegisterBankEmitter::run(raw_ostream &OS) {
+  std::vector<Record*> Targets = Records.getAllDerivedDefinitions("Target");
+  if (Targets.size() != 1)
----------------
ab wrote:
> <Record *>
Ok


================
Comment at: utils/TableGen/RegisterBankEmitter.cpp:362
+
+  OS << "// This file is generated. Do not edit.\n\n";
+  emitHeader(OS, TargetName, Banks);
----------------
ab wrote:
> emitSourceFileHeader ?
Ok


https://reviews.llvm.org/D27338