[PATCH] Fix type error of 'switch (SUBREG_BYTE (op)).'

[PATCH] Fix type error of 'switch (SUBREG_BYTE (op)).'

[Jin Ma]

For example:
(define_insn "mov_lowpart_sidi2"
  [(set (match_operand:SI            0 "register_operand" "=r")
        (subreg:SI (match_operand:DI 1 "register_operand" " r") 0))]
  "TARGET_64BIT"
  "mov\t%0,%1")

(define_insn "mov_highpart_sidi2"
  [(set (match_operand:SI            0 "register_operand" "=r")
        (subreg:SI (match_operand:DI 1 "register_operand" " r") 1))]
  "TARGET_64BIT"
  "movh\t%0,%1")

When defining the above patterns, the generated file insn-recog.cc will
appear 'switch (SUBREG_BYTE (op))', but since the return value of
SUBREG_BYTE is poly_uint16_pod, the following error will occur:
"error: switch quantity not an integer".

gcc/ChangeLog:

	* genrecog.cc (print_nonbool_test): Fix type error of
	'switch (SUBREG_BYTE (op))'.
---
 gcc/genrecog.cc | 1 +
 1 file changed, 1 insertion(+)

diff --git a/gcc/genrecog.cc b/gcc/genrecog.cc
index 6dd375da5e3..04a5533ca4b 100644
--- a/gcc/genrecog.cc
+++ b/gcc/genrecog.cc
@@ -4619,6 +4619,7 @@ print_nonbool_test (output_state *os, const rtx_test &test)
       printf ("SUBREG_BYTE (");
       print_test_rtx (os, test);
       printf (")");
+      printf (".to_constant ()");
       break;
 
     case rtx_test::WIDE_INT_FIELD:
-- 
2.17.1

Re: [PATCH] Fix type error of 'switch (SUBREG_BYTE (op)).'

[Jeff Law]

On 5/17/23 03:03, Jin Ma wrote:
> For example:
> (define_insn "mov_lowpart_sidi2"
>    [(set (match_operand:SI            0 "register_operand" "=r")
>          (subreg:SI (match_operand:DI 1 "register_operand" " r") 0))]
>    "TARGET_64BIT"
>    "mov\t%0,%1")
> 
> (define_insn "mov_highpart_sidi2"
>    [(set (match_operand:SI            0 "register_operand" "=r")
>          (subreg:SI (match_operand:DI 1 "register_operand" " r") 1))]
>    "TARGET_64BIT"
>    "movh\t%0,%1")
> 
> When defining the above patterns, the generated file insn-recog.cc will
> appear 'switch (SUBREG_BYTE (op))', but since the return value of
> SUBREG_BYTE is poly_uint16_pod, the following error will occur:
> "error: switch quantity not an integer".
> 
> gcc/ChangeLog:
> 
> 	* genrecog.cc (print_nonbool_test): Fix type error of
> 	'switch (SUBREG_BYTE (op))'.
Thanks.  Installed.

jeff

Re: [PATCH] Fix type error of 'switch (SUBREG_BYTE (op)).'

[Richard Sandiford]

Jeff Law via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> On 5/17/23 03:03, Jin Ma wrote:
>> For example:
>> (define_insn "mov_lowpart_sidi2"
>>    [(set (match_operand:SI            0 "register_operand" "=r")
>>          (subreg:SI (match_operand:DI 1 "register_operand" " r") 0))]
>>    "TARGET_64BIT"
>>    "mov\t%0,%1")
>> 
>> (define_insn "mov_highpart_sidi2"
>>    [(set (match_operand:SI            0 "register_operand" "=r")
>>          (subreg:SI (match_operand:DI 1 "register_operand" " r") 1))]
>>    "TARGET_64BIT"
>>    "movh\t%0,%1")
>> 
>> When defining the above patterns, the generated file insn-recog.cc will
>> appear 'switch (SUBREG_BYTE (op))', but since the return value of
>> SUBREG_BYTE is poly_uint16_pod, the following error will occur:
>> "error: switch quantity not an integer".
>> 
>> gcc/ChangeLog:
>> 
>> 	* genrecog.cc (print_nonbool_test): Fix type error of
>> 	'switch (SUBREG_BYTE (op))'.
> Thanks.  Installed.

We shouldn't add to_constant just because it's a convenient
way of getting rid of errors :)  There has to be a good reason
in principle why the value is known at compile time.

So I think this should be reverted.  Nothing guarantees that
SUBREG_BYTEs are constant on AArch64 and RISC-V.  And for SVE
it's common for them not to be.

If we want to support the above, I think we need to make the
generator use known_eq instead.

The patterns don't look right though.  An SI subreg of a DI
can't have a SUBREG_BYTE of 1.  And the lowpart SUBREG_BYTE
depends on endianness.  So I think a better way of writing
the lowpart pattern above is to use subreg_lowpart_operator
(which riscv already has).

The high part can't be done using subregs though.

Thanks,
Richard

Re: [PATCH] Fix type error of 'switch (SUBREG_BYTE (op)).'

[Jin Ma]

> > On 5/17/23 03:03, Jin Ma wrote:
> >> For example:
> >> (define_insn "mov_lowpart_sidi2"
> >>    [(set (match_operand:SI            0 "register_operand" "=r")
> >>          (subreg:SI (match_operand:DI 1 "register_operand" " r") 0))]
> >>    "TARGET_64BIT"
> >>    "mov\t%0,%1")
> >> 
> >> (define_insn "mov_highpart_sidi2"
> >>    [(set (match_operand:SI            0 "register_operand" "=r")
> >>          (subreg:SI (match_operand:DI 1 "register_operand" " r") 1))]
> >>    "TARGET_64BIT"
> >>    "movh\t%0,%1")
> >> 
> >> When defining the above patterns, the generated file insn-recog.cc will
> >> appear 'switch (SUBREG_BYTE (op))', but since the return value of
> >> SUBREG_BYTE is poly_uint16_pod, the following error will occur:
> >> "error: switch quantity not an integer".
> >> 
> >> gcc/ChangeLog:
> >> 
> >>  * genrecog.cc (print_nonbool_test): Fix type error of
> >>  'switch (SUBREG_BYTE (op))'.
> > Thanks.  Installed.
> 
> We shouldn't add to_constant just because it's a convenient
> way of getting rid of errors :)  There has to be a good reason
> in principle why the value is known at compile time.
> 
> So I think this should be reverted.  Nothing guarantees that
> SUBREG_BYTEs are constant on AArch64 and RISC-V.  And for SVE
> it's common for them not to be.
> 
> If we want to support the above, I think we need to make the
> generator use known_eq instead.
> 
> The patterns don't look right though.  An SI subreg of a DI
> can't have a SUBREG_BYTE of 1.  And the lowpart SUBREG_BYTE
> depends on endianness.  So I think a better way of writing
> the lowpart pattern above is to use subreg_lowpart_operator
> (which riscv already has).
> 
> The high part can't be done using subregs though.
> 
> Thanks,
> Richard

I'm trying to understand what you mean. The return value of
the SUBREG_BYTE is poly_uint16_pod. When the value of the
NUM_POLY_INT_COEFFS is 2, using to_constant () to convert
to a constant may lose coeffs[1], right?

I agree with this, we can revert this first, but this problem
always exists, and it seems that we need to find other suitable
ways to solve this error.

Thanks,
Jin

Re: [PATCH] Fix type error of 'switch (SUBREG_BYTE (op)).'

[Richard Sandiford]

"Jin Ma" <jinma@linux.alibaba.com> writes:
>> > On 5/17/23 03:03, Jin Ma wrote:
>> >> For example:
>> >> (define_insn "mov_lowpart_sidi2"
>> >>    [(set (match_operand:SI            0 "register_operand" "=r")
>> >>          (subreg:SI (match_operand:DI 1 "register_operand" " r") 0))]
>> >>    "TARGET_64BIT"
>> >>    "mov\t%0,%1")
>> >> 
>> >> (define_insn "mov_highpart_sidi2"
>> >>    [(set (match_operand:SI            0 "register_operand" "=r")
>> >>          (subreg:SI (match_operand:DI 1 "register_operand" " r") 1))]
>> >>    "TARGET_64BIT"
>> >>    "movh\t%0,%1")
>> >> 
>> >> When defining the above patterns, the generated file insn-recog.cc will
>> >> appear 'switch (SUBREG_BYTE (op))', but since the return value of
>> >> SUBREG_BYTE is poly_uint16_pod, the following error will occur:
>> >> "error: switch quantity not an integer".
>> >> 
>> >> gcc/ChangeLog:
>> >> 
>> >>  * genrecog.cc (print_nonbool_test): Fix type error of
>> >>  'switch (SUBREG_BYTE (op))'.
>> > Thanks.  Installed.
>> 
>> We shouldn't add to_constant just because it's a convenient
>> way of getting rid of errors :)  There has to be a good reason
>> in principle why the value is known at compile time.
>> 
>> So I think this should be reverted.  Nothing guarantees that
>> SUBREG_BYTEs are constant on AArch64 and RISC-V.  And for SVE
>> it's common for them not to be.
>> 
>> If we want to support the above, I think we need to make the
>> generator use known_eq instead.
>> 
>> The patterns don't look right though.  An SI subreg of a DI
>> can't have a SUBREG_BYTE of 1.  And the lowpart SUBREG_BYTE
>> depends on endianness.  So I think a better way of writing
>> the lowpart pattern above is to use subreg_lowpart_operator
>> (which riscv already has).
>> 
>> The high part can't be done using subregs though.
>> 
>> Thanks,
>> Richard
>
> I'm trying to understand what you mean. The return value of
> the SUBREG_BYTE is poly_uint16_pod. When the value of the
> NUM_POLY_INT_COEFFS is 2, using to_constant () to convert
> to a constant may lose coeffs[1], right?

to_constant will abort compilation if coeffs[1] is nonzero.
So we should only use it in contexts where coeffs[1] is already
known to be zero.

We don't know coeffs[1] is already zero here.  The purpose of the
switch statement is to test the value, which is currently unknown.

We handle (subreg ... 0) correctly when all subregs have byte 0.
In that case we use an "if" statement and use known_eq to test
the SUBREG_BYTE.  The problem only occurs when there are multiple
possible SUBREG_BYTEs at the same decision point.

I agree that it would make sense to support this, probably by
emitting a chain of known_eq "if" statements.

But before we do that, I think we should check whether there
is a legitimate use case.  Like I say, the patterns above don't
look correct to me.

Thanks,
Richard

Re: [PATCH] Fix type error of 'switch (SUBREG_BYTE (op)).'

[Jeff Law]

On 5/23/23 06:27, Richard Sandiford wrote:
> Jeff Law via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
>> On 5/17/23 03:03, Jin Ma wrote:
>>> For example:
>>> (define_insn "mov_lowpart_sidi2"
>>>     [(set (match_operand:SI            0 "register_operand" "=r")
>>>           (subreg:SI (match_operand:DI 1 "register_operand" " r") 0))]
>>>     "TARGET_64BIT"
>>>     "mov\t%0,%1")
>>>
>>> (define_insn "mov_highpart_sidi2"
>>>     [(set (match_operand:SI            0 "register_operand" "=r")
>>>           (subreg:SI (match_operand:DI 1 "register_operand" " r") 1))]
>>>     "TARGET_64BIT"
>>>     "movh\t%0,%1")
>>>
>>> When defining the above patterns, the generated file insn-recog.cc will
>>> appear 'switch (SUBREG_BYTE (op))', but since the return value of
>>> SUBREG_BYTE is poly_uint16_pod, the following error will occur:
>>> "error: switch quantity not an integer".
>>>
>>> gcc/ChangeLog:
>>>
>>> 	* genrecog.cc (print_nonbool_test): Fix type error of
>>> 	'switch (SUBREG_BYTE (op))'.
>> Thanks.  Installed.
> 
> We shouldn't add to_constant just because it's a convenient
> way of getting rid of errors :)  There has to be a good reason
> in principle why the value is known at compile time.
Agreed.  I fully expected the constant to be known at compile time.  I 
wasn't aware we had real uses of polys in the SUBREG_BYTE field.

> 
> So I think this should be reverted.  Nothing guarantees that
> SUBREG_BYTEs are constant on AArch64 and RISC-V.  And for SVE
> it's common for them not to be.
That's fine with me.

Jeff