| Message ID | 20230517090315.795-1-jinma@linux.alibaba.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix type error of 'switch (SUBREG_BYTE (op)).' | expand |
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
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
> > 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
"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
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
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:
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(+)