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

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.

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?

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.

-Tom

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