Page MenuHomePhabricator

Virtualize TargetInstrInfo::getRegClass()
ClosedPublic

Authored by rampitec on Jun 12 2019, 7:45 AM.

Details

Summary

AMDGPU target needs to override getRegClass() used during
instruction selection. We now may have either 32 or 64 bit
conditional registers used in the same instructions. For
that purpose special SReg_1 register class is created which
is dynamically resolved to either SReg_64 or SGPR_32 depending
on the subtarget attributes.

See AMDGPU specific change D63204 for the usage.

Diff Detail

Repository
rL LLVM

Event Timeline

rampitec created this revision.Jun 12 2019, 7:45 AM

Maybe I'm missing something, but the override implemented in D63204 looks like a subset of the default TII::getRegClass that falls through to TRI::getRegClass, just like the default does. Is this change actually necessary?

Maybe I'm missing something, but the override implemented in D63204 looks like a subset of the default TII::getRegClass that falls through to TRI::getRegClass, just like the default does. Is this change actually necessary?

It will fallback to TRI::getRegClass(), but to the base class of it through the pointer to TRI in the InstrEmitter, so call goes to the default implementation. Either TII::getRegClass() needs to be virtual or TRI::getRegClass().
I have preferred to override in TII because it is called less frequently than from TRI.

Maybe I'm missing something, but the override implemented in D63204 looks like a subset of the default TII::getRegClass that falls through to TRI::getRegClass, just like the default does. Is this change actually necessary?

It will fallback to TRI::getRegClass(), but to the base class of it through the pointer to TRI in the InstrEmitter, so call goes to the default implementation. Either TII::getRegClass() needs to be virtual or TRI::getRegClass().
I have preferred to override in TII because it is called less frequently than from TRI.

I.e. without override it ends up in this implementation of TargetRegisterInfo::getRegClass(): https://llvm.org/doxygen/TargetRegisterInfo_8h_source.html#l00700
With the override it ends up here: http://llvm.org/doxygen/SIRegisterInfo_8cpp_source.html#l01727
Since it is only needed at selection the override is in the TII to make it lighter. An override in TRI would equally work, but will unnecessary result in more virtual calls than needed.

This revision is now accepted and ready to land.Jun 19 2019, 2:08 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 20 2019, 7:56 AM