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

[Ahmed Bougacha via Phabricator via llvm-commits]

ab added a comment.

In https://reviews.llvm.org/D27338#619757, @dsanders wrote:

> 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.


Yeah, I was worried about that too, but I figured we'll have to generalize that logic at some point anyway, so we might as well clean up the C++ and have a drop-in NFC tablegen replacement.

>> - 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.

SGTM, let's reevaluate once .def is gone.

>> - 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.

Sure, I'll rebase my patches on top and will copy the regbank code somewhere else for now.



================
Comment at: lib/Target/AArch64/AArch64RegisterBankInfo.cpp:46
   AlreadyInit = true;
-  // Initialize the GPR bank.
-  createRegisterBank(AArch64::GPRRegBankID, "GPR");
-  // The GPR register bank is fully defined by all the registers in
-  // GR64all + its subclasses.
-  addRegBankCoverage(AArch64::GPRRegBankID, AArch64::GPR64allRegClassID, TRI);
+  initGeneratedInfo(TRI);
+// Now, the content.
----------------
How about:
- introduce an intermediate class, 'AArch64GenRegisterBankInfo'
- make the RegBanks array a member
- replace initGeneratedInfo with the ctor
- remove the AlreadyInit logic
- consolidate the now-redundant GET_REGBANK_*IFACE* into a single GET_REGBANK_IMPLEMENTATION to avoid having everything in the header

WDYT?


https://reviews.llvm.org/D27338