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

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet requested changes to this revision.
qcolombet added a comment.
This revision now requires changes to proceed.

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



================
Comment at: include/llvm/CodeGen/GlobalISel/RegisterBank.h:38
   const char *Name;
-  unsigned Size;
+  DataTy Data;
   BitVector ContainedRegClasses;
----------------
What's the use for this indirection?


================
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);
----------------
What does CoveredClasses represent?


================
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,
----------------
Why not directly use uint32_t *?
In particular, if we have target with more than 64 classes the API falls apart.


================
Comment at: lib/Target/AArch64/AArch64RegisterBankInfo.cpp:64
+                         (1ull << AArch64::tcGPR64RegClassID) |
+                         (1ull << AArch64::GPR64sponlyRegClassID),
+                     TRI);
----------------
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.


================
Comment at: lib/Target/AArch64/AArch64RegisterBankInfo.cpp:116
+          (1ull << AArch64::
+               QQQQ_with_qsub0_in_FPR128_lo_and_QQQQ_with_qsub3_in_FPR128_loRegClassID),
+      TRI);
----------------
Indeed, I don't want to check if this is correct :S


https://reviews.llvm.org/D27807