[PATCH] D27807: [globalisel] Initialize RegisterBanks with static data. Part 1 of 2.

[Daniel Sanders via Phabricator via llvm-commits]

dsanders added a comment.

In https://reviews.llvm.org/D27807#627299, @qcolombet wrote:

> Hi Daniel,
>
> Unless you can find a more elegant way for the intermediate step, I would push the change from BitSet to uint32_t* as part of the TableGen.


(I assume you mean BitVector rather than BitSet)

The problem with this is that BitVector needs to know its size in advance and this information is obtained from the TRI which doesn't exist at initialization-time.

> That being said, I think we could use intermediate steps:
> 
> 1. Provide methods that perform the current add coverage logic, while producing the result in a dynamically allocated uint32_t
> 2. Gradually switch the dynamic array to static ones
> 3. Generate the static array with TableGen
> 
>   Step #2 and #3 could be merged.
> 
>   Cheers, -Quentin

That was my first approach but I was asked to create the static tables in advance and follow it with an NFC tablegen patch.



================
Comment at: include/llvm/CodeGen/GlobalISel/RegisterBank.h:38
   const char *Name;
-  unsigned Size;
+  DataTy Data;
   BitVector ContainedRegClasses;
----------------
qcolombet wrote:
> What's the use for this indirection?
Tablegen is going to generate a big table of static register bank data and `DataTy` represents an element of that table. The elements appear in this patch (see `GPRData`) and the `<BankName>Data` members are merged into a single table in D27808.

Eventually, `RegisterBank::Data` will just be a reference to that static table. I was going to do it in the tablegen patch but there's no technical reason I can't make that change in this patch if you prefer.


================
Comment at: include/llvm/CodeGen/GlobalISel/RegisterBankInfo.h:410
+  void setRegBankData(unsigned ID, const RegisterBank::DataTy &Data);
+  void setRegBankCoverage(unsigned ID, uint64_t CoveredClasses,
                           const TargetRegisterInfo &TRI);
----------------
qcolombet wrote:
> What does CoveredClasses represent?
It's the classes that addRegBankCoverage() used to add to the register bank.


================
Comment at: lib/CodeGen/GlobalISel/RegisterBankInfo.cpp:99
+void RegisterBankInfo::setRegBankCoverage(unsigned ID, uint64_t CoveredClasses,
                                           const TargetRegisterInfo &TRI) {
+  uint32_t Mask[] = {(uint32_t)CoveredClasses,
----------------
qcolombet wrote:
> Why not directly use uint32_t *?
> In particular, if we have target with more than 64 classes the API falls apart.
I wasn't expecting to hit this before the tablegen patch landed and there was no longer a need for an API. However, the ARM target now has GlobalISel and has just under 100 classes so I hit this yesterday while rebasing. It's uint32_t* now.


================
Comment at: lib/Target/AArch64/AArch64RegisterBankInfo.cpp:64
+                         (1ull << AArch64::tcGPR64RegClassID) |
+                         (1ull << AArch64::GPR64sponlyRegClassID),
+                     TRI);
----------------
qcolombet wrote:
> I think I would rather have the change in representation be part of the TableGen change, because the intermediate step is painful to read and write IMO.
I think I'm getting mutually exclusive comments. On the one hand I'm being asked to refactor up front so that the tablegen patch is NFC but on the other hand I'm being asked to defer that refactoring to the tablegen patch.

I agree that this is ugly but it's not needed for long and there are ways to avoid maintaining it manually. If you call addRegBankCoverage() then it will dump the classes before aborting and it's a simple transformation to convert the class list to this expression. I'm happy to maintain the patch series until they're all ready and commit them in quick succession if that helps.


https://reviews.llvm.org/D27807