llvm dev - May 2017 - Bug in TableGen RegisterBankEmitter

On 05/16/2017 11:57 AM, Daniel Sanders wrote:
>> If that's right, one possible fix would be to rename some of the
subregister indices but that's likely to be quite painful. I'll have a
think and see if I can come up with something nicer.
> 
> I haven't been able to come up with a better answer for this, just an
alternate choice as to where the complexity is. If we were to change the second
argument of RegisterBank<> to a dag then we could provide a choice of
following the class-with-subregs rule or not. This would allow us to prevent it
from following the subreg indices into the wrong classes but it would also make
it harder to define the register banks.
> 
I'm a little confused about what the issue is.  AMDGPU has 2 64-bit register
classes each with sub0 and sub1 sub-registers:

VReg_64:sub0=VGPR_32
VReg_64:sub1=VGPR_32

SReg_64:sub0=SGPR_32
SReg_64:sub1=SGPR_32

Are you saying that tablegen considers VReg_64:sub0 and SReg_64:sub0 to be
the same sub-register class because they are both called sub0 ?

-Tom

>> On 10 May 2017, at 21:58, Daniel Sanders via llvm-dev <llvm-dev at
lists.llvm.org <mailto:llvm-dev at lists.llvm.org>> wrote:
>>
>> Hi Tom,
>>
>> The output:
>> Added VReg_64(explicit)
>> Added VS_32(explicit (VS_32) VReg_64 class-with-subregs: VReg_64)
>> is saying that VS_32 was added because VReg_64 was explicitly specified
and that while inspecting VS_32, it noticed that every register in VS_32 was a
subregister of a register from VReg_64 using a single common subregister index.
>>
>> I've added some more tracing to my local copy and it appears that
the subregister index it found in common was sub0. On the next line, (not shown
above) it also reports that they had sub1 in common too. I think the cause is
that AMDGPU is re-using the same subregister index for unrelated registers and
that tablegen assumes this means that they are compatible.
>>
>> If that's right, one possible fix would be to rename some of the
subregister indices but that's likely to be quite painful. I'll have a
think and see if I can come up with something nicer.
>>
>>> On 10 May 2017, at 16:58, Quentin Colombet <qcolombet at
apple.com <mailto:qcolombet at apple.com>> wrote:
>>>
>>> Hi Tom,
>>>
>>>> On May 10, 2017, at 7:15 AM, Tom Stellard via llvm-dev
<llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>>
wrote:
>>>>
>>>> Hi,
>>>>
>>>> I've run into an issue with the RegisterBankEmitter on the
AMDGPU backend.
>>>> AMDGPU has a register class: VS_32, which is non-allocatable
and contains
>>>> registers from both defined register banks (SGPRRegBank and
VGPRRegBank).
>>>>
>>>> The RegisterBankEmitter is adding this class to the
CoverageData array
>>>> for both register classes, because it contains sub-registers of
one
>>>> of the classes explicitly added to the RegisterBank, for
example:
>>>>
>>>> Added VS_32(explicit (VS_32) VReg_64 class-with-subregs:
VReg_64)
>>>>
>>>> This is a problem, because both RegisterBanks think they cover
>>>> VS_32, even though neither of them actually do.
>>>
>>> I agree this is a bug in the emitter. It should only add the
subclasses of VS_32 that projects to V (resp. S) for the given subregs.
>>>
>>> I let Daniel comment further.
>>>
>>> Cheers,
>>> -Quentin
>>>
>>>>
>>>> What exactly is the best way to fix this?  It seems like we
need some
>>>> additional checks in the RegisterBankEmitter to fix this, but
it's not
>>>> clear to me what we should be checking for.
>>>>
>>>> Thanks,
>>>> Tom
>>>> _______________________________________________
>>>> LLVM Developers mailing list
>>>> llvm-dev at lists.llvm.org <mailto:llvm-dev at
lists.llvm.org>
>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>
>> _______________________________________________
>> LLVM Developers mailing list
>> llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>

> On 16 May 2017, at 18:59, Tom Stellard <tstellar at redhat.com>
wrote:
> 
> On 05/16/2017 11:57 AM, Daniel Sanders wrote:
>>> If that's right, one possible fix would be to rename some of
the subregister indices but that's likely to be quite painful. I'll have
a think and see if I can come up with something nicer.
>> 
>> I haven't been able to come up with a better answer for this, just
an alternate choice as to where the complexity is. If we were to change the
second argument of RegisterBank<> to a dag then we could provide a choice
of following the class-with-subregs rule or not. This would allow us to prevent
it from following the subreg indices into the wrong classes but it would also
make it harder to define the register banks.
>> 
> 
> I'm a little confused about what the issue is.  AMDGPU has 2 64-bit
register
> classes each with sub0 and sub1 sub-registers:
> 
> VReg_64:sub0=VGPR_32
> VReg_64:sub1=VGPR_32
> 
> SReg_64:sub0=SGPR_32
> SReg_64:sub1=SGPR_32
> 
> Are you saying that tablegen considers VReg_64:sub0 and SReg_64:sub0 to be
> the same sub-register class because they are both called sub0 ?
> 
> -Tom
I think you've got the right idea but just to re-phrase it in line with the
way tablegen is looking at it: It's incorrectly thinking that because
VReg_64 has sub-registers via sub0 and SGPR_32 has super-registers via sub0,
that VReg_64 must overlap SGPR_32. It therefore thinks that VGPRRegisterBank
covers SGPR_32.

Tomorrow, I'll dig into the tablegen code and see if we can fix it on that
side. I expect that will involve individually checking each register which
isn't ideal but I think it can be made to work.
>>> On 10 May 2017, at 21:58, Daniel Sanders via llvm-dev <llvm-dev
at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>> wrote:
>>> 
>>> Hi Tom,
>>> 
>>> The output:
>>> Added VReg_64(explicit)
>>> Added VS_32(explicit (VS_32) VReg_64 class-with-subregs: VReg_64)
>>> is saying that VS_32 was added because VReg_64 was explicitly
specified and that while inspecting VS_32, it noticed that every register in
VS_32 was a subregister of a register from VReg_64 using a single common
subregister index.
>>> 
>>> I've added some more tracing to my local copy and it appears
that the subregister index it found in common was sub0. On the next line, (not
shown above) it also reports that they had sub1 in common too. I think the cause
is that AMDGPU is re-using the same subregister index for unrelated registers
and that tablegen assumes this means that they are compatible.
>>> 
>>> If that's right, one possible fix would be to rename some of
the subregister indices but that's likely to be quite painful. I'll have
a think and see if I can come up with something nicer.
>>> 
>>>> On 10 May 2017, at 16:58, Quentin Colombet <qcolombet at
apple.com <mailto:qcolombet at apple.com>> wrote:
>>>> 
>>>> Hi Tom,
>>>> 
>>>>> On May 10, 2017, at 7:15 AM, Tom Stellard via llvm-dev
<llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>>
wrote:
>>>>> 
>>>>> Hi,
>>>>> 
>>>>> I've run into an issue with the RegisterBankEmitter on
the AMDGPU backend.
>>>>> AMDGPU has a register class: VS_32, which is
non-allocatable and contains
>>>>> registers from both defined register banks (SGPRRegBank and
VGPRRegBank).
>>>>> 
>>>>> The RegisterBankEmitter is adding this class to the
CoverageData array
>>>>> for both register classes, because it contains
sub-registers of one
>>>>> of the classes explicitly added to the RegisterBank, for
example:
>>>>> 
>>>>> Added VS_32(explicit (VS_32) VReg_64 class-with-subregs:
VReg_64)
>>>>> 
>>>>> This is a problem, because both RegisterBanks think they
cover
>>>>> VS_32, even though neither of them actually do.
>>>> 
>>>> I agree this is a bug in the emitter. It should only add the
subclasses of VS_32 that projects to V (resp. S) for the given subregs.
>>>> 
>>>> I let Daniel comment further.
>>>> 
>>>> Cheers,
>>>> -Quentin
>>>> 
>>>>> 
>>>>> What exactly is the best way to fix this?  It seems like we
need some
>>>>> additional checks in the RegisterBankEmitter to fix this,
but it's not
>>>>> clear to me what we should be checking for.
>>>>> 
>>>>> Thanks,
>>>>> Tom
>>>>> _______________________________________________
>>>>> LLVM Developers mailing list
>>>>> llvm-dev at lists.llvm.org <mailto:llvm-dev at
lists.llvm.org>
>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>> 
>>> _______________________________________________
>>> LLVM Developers mailing list
>>> llvm-dev at lists.llvm.org <mailto:llvm-dev at
lists.llvm.org>
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>> 
>

Hi Tom,

I believe I have a fix for this in https://reviews.llvm.org/D33352
<https://reviews.llvm.org/D33352>.
> On 16 May 2017, at 19:58, Daniel Sanders via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 
>> 
>> On 16 May 2017, at 18:59, Tom Stellard <tstellar at redhat.com>
wrote:
>> 
>> On 05/16/2017 11:57 AM, Daniel Sanders wrote:
>>>> If that's right, one possible fix would be to rename some
of the subregister indices but that's likely to be quite painful. I'll
have a think and see if I can come up with something nicer.
>>> 
>>> I haven't been able to come up with a better answer for this,
just an alternate choice as to where the complexity is. If we were to change the
second argument of RegisterBank<> to a dag then we could provide a choice
of following the class-with-subregs rule or not. This would allow us to prevent
it from following the subreg indices into the wrong classes but it would also
make it harder to define the register banks.
>>> 
>> 
>> I'm a little confused about what the issue is.  AMDGPU has 2 64-bit
register
>> classes each with sub0 and sub1 sub-registers:
>> 
>> VReg_64:sub0=VGPR_32
>> VReg_64:sub1=VGPR_32
>> 
>> SReg_64:sub0=SGPR_32
>> SReg_64:sub1=SGPR_32
>> 
>> Are you saying that tablegen considers VReg_64:sub0 and SReg_64:sub0 to
be
>> the same sub-register class because they are both called sub0 ?
>> 
>> -Tom
> 
> I think you've got the right idea but just to re-phrase it in line with
the way tablegen is looking at it: It's incorrectly thinking that because
VReg_64 has sub-registers via sub0 and SGPR_32 has super-registers via sub0,
that VReg_64 must overlap SGPR_32. It therefore thinks that VGPRRegisterBank
covers SGPR_32.
> 
> Tomorrow, I'll dig into the tablegen code and see if we can fix it on
that side. I expect that will involve individually checking each register which
isn't ideal but I think it can be made to work.
> 
>>>> On 10 May 2017, at 21:58, Daniel Sanders via llvm-dev
<llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>>
wrote:
>>>> 
>>>> Hi Tom,
>>>> 
>>>> The output:
>>>> Added VReg_64(explicit)
>>>> Added VS_32(explicit (VS_32) VReg_64 class-with-subregs:
VReg_64)
>>>> is saying that VS_32 was added because VReg_64 was explicitly
specified and that while inspecting VS_32, it noticed that every register in
VS_32 was a subregister of a register from VReg_64 using a single common
subregister index.
>>>> 
>>>> I've added some more tracing to my local copy and it
appears that the subregister index it found in common was sub0. On the next
line, (not shown above) it also reports that they had sub1 in common too. I
think the cause is that AMDGPU is re-using the same subregister index for
unrelated registers and that tablegen assumes this means that they are
compatible.
>>>> 
>>>> If that's right, one possible fix would be to rename some
of the subregister indices but that's likely to be quite painful. I'll
have a think and see if I can come up with something nicer.
>>>> 
>>>>> On 10 May 2017, at 16:58, Quentin Colombet <qcolombet at
apple.com <mailto:qcolombet at apple.com>> wrote:
>>>>> 
>>>>> Hi Tom,
>>>>> 
>>>>>> On May 10, 2017, at 7:15 AM, Tom Stellard via llvm-dev
<llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>>
wrote:
>>>>>> 
>>>>>> Hi,
>>>>>> 
>>>>>> I've run into an issue with the RegisterBankEmitter
on the AMDGPU backend.
>>>>>> AMDGPU has a register class: VS_32, which is
non-allocatable and contains
>>>>>> registers from both defined register banks (SGPRRegBank
and VGPRRegBank).
>>>>>> 
>>>>>> The RegisterBankEmitter is adding this class to the
CoverageData array
>>>>>> for both register classes, because it contains
sub-registers of one
>>>>>> of the classes explicitly added to the RegisterBank,
for example:
>>>>>> 
>>>>>> Added VS_32(explicit (VS_32) VReg_64
class-with-subregs: VReg_64)
>>>>>> 
>>>>>> This is a problem, because both RegisterBanks think
they cover
>>>>>> VS_32, even though neither of them actually do.
>>>>> 
>>>>> I agree this is a bug in the emitter. It should only add
the subclasses of VS_32 that projects to V (resp. S) for the given subregs.
>>>>> 
>>>>> I let Daniel comment further.
>>>>> 
>>>>> Cheers,
>>>>> -Quentin
>>>>> 
>>>>>> 
>>>>>> What exactly is the best way to fix this?  It seems
like we need some
>>>>>> additional checks in the RegisterBankEmitter to fix
this, but it's not
>>>>>> clear to me what we should be checking for.
>>>>>> 
>>>>>> Thanks,
>>>>>> Tom
>>>>>> _______________________________________________
>>>>>> LLVM Developers mailing list
>>>>>> llvm-dev at lists.llvm.org <mailto:llvm-dev at
lists.llvm.org>
>>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>>> 
>>>> _______________________________________________
>>>> LLVM Developers mailing list
>>>> llvm-dev at lists.llvm.org <mailto:llvm-dev at
lists.llvm.org>
>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>> 
>> 
> 
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
<http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20170519/73dcca12/attachment.html>