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

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added inline comments.


================
Comment at: lib/Target/AArch64/AArch64GenRegisterBankInfo.def:89
+    {nullptr, 1},
+    {nullptr, 1},
+    // 21: GPR 32-bit value to FPR 32-bit value. <-- This must match
----------------
dsanders wrote:
> qcolombet wrote:
> > dsanders wrote:
> > > qcolombet wrote:
> > > > Why do we need three of them instead of none previously?
> > > > 
> > > > At the very least I would have expected that we need just one of this input.
> > > We should have had the three of them at the end of the table but got away without them because FPR was last and the missing elements weren't accessed. Now that FPR is first we need them to pad GPR's mappings out to the right indices.
> > > 
> > > I've added comments indicating the invalid mappings they correspond to.
> > Can't we update the indices or formulae so that we don't need those invalid entries at all?
> > If that's too complicated, I am fine with moving along with the current version. It will be table generated as some point any way.
> It doesn't look too complicated. We can stop getCopyMapping() needing the invalid entries by dropping the relevant size to index-offset mappings from getRegBankBaseIdxOffset(). However, getValueMapping() also uses that so we'd need to duplicate the function first before trimming the getCopyMapping() version.
> 
> It would be easiest for me to do this as a follow-up patch but I can insert it into the series if you prefer.
Just move forward with the current version. It'll get clean up with the related tablegen support when we add that eventually.


https://reviews.llvm.org/D27338