[llvm-dev] [GlobalISel] Can we drop RegisterBankInfo::getInstrAlternativeMappings() ?

[Quentin Colombet via llvm-dev]

Hi Tom,

> On Jul 28, 2016, at 1:15 PM, Tom Stellard <tom at stellard.net> wrote:
> 
> Hi,
> 
> I've spent some time playing around with GlobalISel on the AMDGPU target, and I
> was wondering if there is any reason to have
> RegisterBankInfo::getInstrAlternativeMappings() and
> RegisterBankInfo::getInstrMapping() as separate functions.

Yes, there is!
At first I thought about having just one getInstrMappings method where the first element in the list would be the default. I decided against it for performance reason. When we run the fast mode of regbank select, we know we will always take the first element in the list, thus there is no point for the target to have to populate the list with alternative mappings.
Moreover, I wanted to have both methods separated to enforce the “first-element-is-the-default” rule. Since that part is done in a target independent fashion, we avoid the chance of screwing up when doing the targeting.

Ultimately, those could be tablegen and we will reconsider, but in the meantime, I believe the distinction makes sense.

> 
> Could we instead replace these two functions with just one:
> RegisterBankInfo::getInstrMappings() and then just treat the first mapping
> in the list as the 'default' mapping to use for 'Fast' RegBankSelect mode?

See my previous answer :).

> 
> The reason this would make sense (at least for the AMDGPU target) is
> because both functions need to do the exact same analysis in order
> to compute the cost and order for the InstrMappings, so we end up
> with a lot of duplicated computations.

I believe you could write the code such that both implementation share the same code like:

getInstrMapping() {
  // here you don’t push the alternative/stop when you get the default
MyMappingImpl(/*onlygetDefaultAndSkipAlt*/ true);
}

getInstrAlternativeMappings() {
  // here you skip the default and return only the alternative
  MyMappingImpl(/*onlygetDefaultAndSkipAlt*/ false);
}

That being said, my recommendation would be:
- keep the default simple: this is for compile time.
- alternatives are optional: this is for optimization.

Cheers,
-Quentin

> 
> -Tom