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

[Daniel Sanders via Phabricator via llvm-commits]

dsanders added a comment.

>>> 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.
>> 
>> ! In https://reviews.llvm.org/D27807#628051, @qcolombet wrote:
>> 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.
> 
> IIRC, we get this information statically when we do the generation of TargetRegisterInfo. I suspect we would need to refactor that code to reuse it here, but in the end, everything should be static and this finishInit should not be here.

Yes, the information is available to tablegen but we don't have our tablegen pass until later in the patch series so we can't make use of that just yet. At the moment, the number of classes is only available to the backend after <Target>Subtarget has been constructed which is after the point that the table is initialized.

Would you be ok with this patch if I eliminate the finishInit() function in https://reviews.llvm.org/D27338 and make sure they are committed close together so we're not in this halfway state for long?

>>> 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.
> 
> I believe the NFC approach is not incompatible with a change of representation in the same patch.

Sorry, I've been confusing here by mis-using 'NFC'. I meant that the plan was to re-factor into the desired implementation and then have a tablegen patch emit the same implementation.

For reference, the plan Ahmed suggested on https://reviews.llvm.org/D27338 was:

- 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

My current patch series isn't completely in line with this at the moment (in particular AlreadyInit hasn't been removed yet) but this is the general direction I've been aiming for.

If we do switch to your plan, how would you construct the BitVectors for ARM. ARM has ~100 classes so I'd add BitVector(unsigned Size, uint32_t *Bits) but that brings us back to converting the BitVectors to uint32_t* before we tablegen the table.


https://reviews.llvm.org/D27807