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

[Quentin Colombet via llvm-commits]

> On Dec 21, 2016, at 10:50 AM, Daniel Sanders via Phabricator <reviews at reviews.llvm.org> wrote:
> 
> 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?

Totally.

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

To be concrete here I would:
-  Replace BitVector by std::unique_ptr<uint32_t>
- Dynamically allocate this uint32_t array in the ctor (the size is statically known by target)
- Change the implementation of addRegBankCoverage to populate that array with the logic that still add the sub register classes

Then the tablegen patch would:
- Turn the dynamic array into a static one, already populate

To be fair, I would be fine with the current approach as long as it does not live long. The reason is I don’t want to manually have to set all the sub classes (nor check if they are correct), this is too error prone, hence the dynamic logic in the first place.

Cheers,
-Quentin

> 
> 
> https://reviews.llvm.org/D27807
> 
> 
>