* [PATCH] genemit: Handle `const_double_zero' rtx
@ 2020-12-15 20:38 Maciej W. Rozycki
2020-12-15 23:20 ` Jeff Law
0 siblings, 1 reply; 10+ messages in thread
From: Maciej W. Rozycki @ 2020-12-15 20:38 UTC (permalink / raw)
To: gcc-patches; +Cc: Jeff Law, Paul Koning, Martin Liška
Complement commit 20ab43b5cad6 ("RTL: Add `const_double_zero' syntactic
rtx") and remove a commit c60d0736dff7 ("PDP11: Use `const_double_zero'
to express double zero constant") build regression observed with the
`pdp11-aout' target:
genemit: Internal error: abort in gen_exp, at genemit.c:202
make[2]: *** [Makefile:2427: s-emit] Error 1
where a:
(const_double 0 [0] 0 [0] 0 [0] 0 [0])
rtx coming from:
(parallel [
(set (reg:CC 16)
(compare:CC (abs:DF (match_operand:DF 1 ("general_operand") ("0,0")))
(const_double 0 [0] 0 [0] 0 [0] 0 [0])))
(set (match_operand:DF 0 ("nonimmediate_operand") ("=fR,Q"))
(abs:DF (match_dup 1)))
])
and ultimately `(const_double_zero)' referred in a named RTL insn cannot
be interpreted. Handle the rtx then by supplying the constant 0 double
operand requested, resulting in the following update to insn-emit.c code
produced for the `pdp11-aout' target, relative to before the triggering
commit:
@@ -1514,7 +1514,7 @@ gen_absdf2_cc (rtx operand0 ATTRIBUTE_UN
gen_rtx_COMPARE (CCmode,
gen_rtx_ABS (DFmode,
operand1),
- const0_rtx)),
+ CONST_DOUBLE_ATOF ("0", VOIDmode))),
gen_rtx_SET (operand0,
gen_rtx_ABS (DFmode,
copy_rtx (operand1)))));
@@ -1555,7 +1555,7 @@ gen_negdf2_cc (rtx operand0 ATTRIBUTE_UN
gen_rtx_COMPARE (CCmode,
gen_rtx_NEG (DFmode,
operand1),
- const0_rtx)),
+ CONST_DOUBLE_ATOF ("0", VOIDmode))),
gen_rtx_SET (operand0,
gen_rtx_NEG (DFmode,
copy_rtx (operand1)))));
@@ -1790,7 +1790,7 @@ gen_muldf3_cc (rtx operand0 ATTRIBUTE_UN
gen_rtx_MULT (DFmode,
operand1,
operand2),
- const0_rtx)),
+ CONST_DOUBLE_ATOF ("0", VOIDmode))),
gen_rtx_SET (operand0,
gen_rtx_MULT (DFmode,
copy_rtx (operand1),
@@ -1942,7 +1942,7 @@ gen_divdf3_cc (rtx operand0 ATTRIBUTE_UN
gen_rtx_DIV (DFmode,
operand1,
operand2),
- const0_rtx)),
+ CONST_DOUBLE_ATOF ("0", VOIDmode))),
gen_rtx_SET (operand0,
gen_rtx_DIV (DFmode,
copy_rtx (operand1),
gcc/
* genemit.c (gen_exp) <CONST_DOUBLE>: Handle `const_double_zero'
rtx.
---
Hi,
I have verified this with a bootstrap of a `powerpc64le-linux-gnu' native
compiler, and that it builds a `pdp11-aout' cross-compiler up to the point
of failing to find target headers. I have no means to verify it further,
the target configuration is too exotic to me, so I will appreciate help.
NB this only triggers with insn-emit.c code produced from named RTL insns
for the purpose of calling them directly, which does not apply for the VAX
backend, as it does not give names to any insns using `const_double_zero'.
Which is why I didn't spot this issue with all my VAX verification.
Thanks to Martin for bringing my attention to this regression, and sorry
to miss this in testing.
OK to apply?
Maciej
---
gcc/genemit.c | 8 ++++++++
1 file changed, 8 insertions(+)
gcc-genemit-const-double-zero.diff
Index: gcc/gcc/genemit.c
===================================================================
--- gcc.orig/gcc/genemit.c
+++ gcc/gcc/genemit.c
@@ -195,6 +195,14 @@ gen_exp (rtx x, enum rtx_code subroutine
return;
case CONST_DOUBLE:
+ /* Handle `const_double_zero' rtx. */
+ if (CONST_DOUBLE_REAL_VALUE (x)->cl == rvc_zero)
+ {
+ printf ("CONST_DOUBLE_ATOF (\"0\", %smode)",
+ GET_MODE_NAME (GET_MODE (x)));
+ return;
+ }
+ /* Fall through. */
case CONST_FIXED:
case CONST_WIDE_INT:
/* These shouldn't be written in MD files. Instead, the appropriate
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] genemit: Handle `const_double_zero' rtx
2020-12-15 20:38 [PATCH] genemit: Handle `const_double_zero' rtx Maciej W. Rozycki
@ 2020-12-15 23:20 ` Jeff Law
2020-12-16 11:35 ` Maciej W. Rozycki
0 siblings, 1 reply; 10+ messages in thread
From: Jeff Law @ 2020-12-15 23:20 UTC (permalink / raw)
To: Maciej W. Rozycki, gcc-patches; +Cc: Paul Koning, Martin Liška
On 12/15/20 1:38 PM, Maciej W. Rozycki wrote:
> Complement commit 20ab43b5cad6 ("RTL: Add `const_double_zero' syntactic
> rtx") and remove a commit c60d0736dff7 ("PDP11: Use `const_double_zero'
> to express double zero constant") build regression observed with the
> `pdp11-aout' target:
>
> genemit: Internal error: abort in gen_exp, at genemit.c:202
> make[2]: *** [Makefile:2427: s-emit] Error 1
>
> where a:
>
> (const_double 0 [0] 0 [0] 0 [0] 0 [0])
>
> rtx coming from:
>
> (parallel [
> (set (reg:CC 16)
> (compare:CC (abs:DF (match_operand:DF 1 ("general_operand") ("0,0")))
> (const_double 0 [0] 0 [0] 0 [0] 0 [0])))
> (set (match_operand:DF 0 ("nonimmediate_operand") ("=fR,Q"))
> (abs:DF (match_dup 1)))
> ])
>
> and ultimately `(const_double_zero)' referred in a named RTL insn cannot
> be interpreted. Handle the rtx then by supplying the constant 0 double
> operand requested, resulting in the following update to insn-emit.c code
> produced for the `pdp11-aout' target, relative to before the triggering
> commit:
>
> @@ -1514,7 +1514,7 @@ gen_absdf2_cc (rtx operand0 ATTRIBUTE_UN
> gen_rtx_COMPARE (CCmode,
> gen_rtx_ABS (DFmode,
> operand1),
> - const0_rtx)),
> + CONST_DOUBLE_ATOF ("0", VOIDmode))),
> gen_rtx_SET (operand0,
> gen_rtx_ABS (DFmode,
> copy_rtx (operand1)))));
> @@ -1555,7 +1555,7 @@ gen_negdf2_cc (rtx operand0 ATTRIBUTE_UN
> gen_rtx_COMPARE (CCmode,
> gen_rtx_NEG (DFmode,
> operand1),
> - const0_rtx)),
> + CONST_DOUBLE_ATOF ("0", VOIDmode))),
> gen_rtx_SET (operand0,
> gen_rtx_NEG (DFmode,
> copy_rtx (operand1)))));
> @@ -1790,7 +1790,7 @@ gen_muldf3_cc (rtx operand0 ATTRIBUTE_UN
> gen_rtx_MULT (DFmode,
> operand1,
> operand2),
> - const0_rtx)),
> + CONST_DOUBLE_ATOF ("0", VOIDmode))),
> gen_rtx_SET (operand0,
> gen_rtx_MULT (DFmode,
> copy_rtx (operand1),
> @@ -1942,7 +1942,7 @@ gen_divdf3_cc (rtx operand0 ATTRIBUTE_UN
> gen_rtx_DIV (DFmode,
> operand1,
> operand2),
> - const0_rtx)),
> + CONST_DOUBLE_ATOF ("0", VOIDmode))),
> gen_rtx_SET (operand0,
> gen_rtx_DIV (DFmode,
> copy_rtx (operand1),
>
> gcc/
> * genemit.c (gen_exp) <CONST_DOUBLE>: Handle `const_double_zero'
> rtx.
> ---
> Hi,
>
> I have verified this with a bootstrap of a `powerpc64le-linux-gnu' native
> compiler, and that it builds a `pdp11-aout' cross-compiler up to the point
> of failing to find target headers. I have no means to verify it further,
> the target configuration is too exotic to me, so I will appreciate help.
>
> NB this only triggers with insn-emit.c code produced from named RTL insns
> for the purpose of calling them directly, which does not apply for the VAX
> backend, as it does not give names to any insns using `const_double_zero'.
> Which is why I didn't spot this issue with all my VAX verification.
>
> Thanks to Martin for bringing my attention to this regression, and sorry
> to miss this in testing.
>
> OK to apply?
Presumably you can't use CONST0_RTX (mode) here? Assuming that's the
case, then this is fine.
jeff
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] genemit: Handle `const_double_zero' rtx
2020-12-15 23:20 ` Jeff Law
@ 2020-12-16 11:35 ` Maciej W. Rozycki
2020-12-16 12:28 ` Richard Sandiford
2020-12-16 19:19 ` Paul Koning
0 siblings, 2 replies; 10+ messages in thread
From: Maciej W. Rozycki @ 2020-12-16 11:35 UTC (permalink / raw)
To: Jeff Law; +Cc: gcc-patches, Paul Koning, Martin Liška
On Tue, 15 Dec 2020, Jeff Law wrote:
> > @@ -1942,7 +1942,7 @@ gen_divdf3_cc (rtx operand0 ATTRIBUTE_UN
> > gen_rtx_DIV (DFmode,
> > operand1,
> > operand2),
> > - const0_rtx)),
> > + CONST_DOUBLE_ATOF ("0", VOIDmode))),
> > gen_rtx_SET (operand0,
> > gen_rtx_DIV (DFmode,
> > copy_rtx (operand1),
> >
> > gcc/
> > * genemit.c (gen_exp) <CONST_DOUBLE>: Handle `const_double_zero'
> > rtx.
> > ---
> > Hi,
> >
> > I have verified this with a bootstrap of a `powerpc64le-linux-gnu' native
> > compiler, and that it builds a `pdp11-aout' cross-compiler up to the point
> > of failing to find target headers. I have no means to verify it further,
> > the target configuration is too exotic to me, so I will appreciate help.
> >
> > NB this only triggers with insn-emit.c code produced from named RTL insns
> > for the purpose of calling them directly, which does not apply for the VAX
> > backend, as it does not give names to any insns using `const_double_zero'.
> > Which is why I didn't spot this issue with all my VAX verification.
> >
> > Thanks to Martin for bringing my attention to this regression, and sorry
> > to miss this in testing.
> >
> > OK to apply?
> Presumably you can't use CONST0_RTX (mode) here? Assuming that's the
> case, then this is fine.
Well, `mode' is VOID for simplicity and to match what the middle end
presents as an operand to the COMPARE operation, as also shown with the
diff above, so with CONST0_RTX (mode) we'd be back to what amounts to
`const0_rtx' aka `const_int 0', which is exactly what we want to get away
from.
Alternatively we could possibly give `const_double_zero' a proper FP
mode, but it seems to me like an unnecessary complication, as it would
then have to be individually requested and iterated over all the relevant
modes.
Have I missed anything here?
NB the PDP-11 pieces affected here and tripping this assertion are I
believe dead code, as these insns are only ever produced by splitters and
do not appear to be referred by their names. We need this change however
for completeness so that `const_double_zero' can be used in contexts where
an insn actually will be referred by its name.
However the PDP-11 code being dead makes it a bit more difficult to
examine actual consequences of such a change like I have proposed than it
would otherwise be. In these circumstances I think my proposal is safe if
a bit overly cautious.
Maciej
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] genemit: Handle `const_double_zero' rtx
2020-12-16 11:35 ` Maciej W. Rozycki
@ 2020-12-16 12:28 ` Richard Sandiford
2020-12-16 13:30 ` Maciej W. Rozycki
2020-12-16 19:19 ` Paul Koning
1 sibling, 1 reply; 10+ messages in thread
From: Richard Sandiford @ 2020-12-16 12:28 UTC (permalink / raw)
To: Maciej W. Rozycki; +Cc: Jeff Law, Paul Koning, gcc-patches
"Maciej W. Rozycki" <macro@linux-mips.org> writes:
> On Tue, 15 Dec 2020, Jeff Law wrote:
>
>> > @@ -1942,7 +1942,7 @@ gen_divdf3_cc (rtx operand0 ATTRIBUTE_UN
>> > gen_rtx_DIV (DFmode,
>> > operand1,
>> > operand2),
>> > - const0_rtx)),
>> > + CONST_DOUBLE_ATOF ("0", VOIDmode))),
>> > gen_rtx_SET (operand0,
>> > gen_rtx_DIV (DFmode,
>> > copy_rtx (operand1),
>> >
>> > gcc/
>> > * genemit.c (gen_exp) <CONST_DOUBLE>: Handle `const_double_zero'
>> > rtx.
>> > ---
>> > Hi,
>> >
>> > I have verified this with a bootstrap of a `powerpc64le-linux-gnu' native
>> > compiler, and that it builds a `pdp11-aout' cross-compiler up to the point
>> > of failing to find target headers. I have no means to verify it further,
>> > the target configuration is too exotic to me, so I will appreciate help.
>> >
>> > NB this only triggers with insn-emit.c code produced from named RTL insns
>> > for the purpose of calling them directly, which does not apply for the VAX
>> > backend, as it does not give names to any insns using `const_double_zero'.
>> > Which is why I didn't spot this issue with all my VAX verification.
>> >
>> > Thanks to Martin for bringing my attention to this regression, and sorry
>> > to miss this in testing.
>> >
>> > OK to apply?
>> Presumably you can't use CONST0_RTX (mode) here? Assuming that's the
>> case, then this is fine.
>
> Well, `mode' is VOID for simplicity and to match what the middle end
> presents as an operand to the COMPARE operation, as also shown with the
> diff above, so with CONST0_RTX (mode) we'd be back to what amounts to
> `const0_rtx' aka `const_int 0', which is exactly what we want to get away
> from.
>
> Alternatively we could possibly give `const_double_zero' a proper FP
> mode, but it seems to me like an unnecessary complication, as it would
> then have to be individually requested and iterated over all the relevant
> modes.
Generated FP const_double rtxes have to have a proper (non-VOID) mode
though. VOIDmode CONST_DOUBLEs are always (legacy) integers instead
of FP values.
So yeah, giving const_double_zero a proper mode seems like the way to go.
It's technically possible to drop it in a pure matching context, but I'm
not sure how valuable that is in practice, given that other parts of the
matched pattern would usually need to refer to the same mode anyway.
> Have I missed anything here?
>
> NB the PDP-11 pieces affected here and tripping this assertion are I
> believe dead code, as these insns are only ever produced by splitters and
> do not appear to be referred by their names. We need this change however
> for completeness so that `const_double_zero' can be used in contexts where
> an insn actually will be referred by its name.
>
> However the PDP-11 code being dead makes it a bit more difficult to
> examine actual consequences of such a change like I have proposed than it
> would otherwise be. In these circumstances I think my proposal is safe if
> a bit overly cautious.
CONST_DOUBLE_ATOF ("0", VOIDmode) seems malformed though, and I'd expect
it to assert in REAL_MODE_FORMAT (via the format_helper constructor).
I'm not sure the patch is strictly safer than the status quo.
FWIW, I agree with Jeff that this ought to be CONST0_RTX (mode).
Thanks,
Richard
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] genemit: Handle `const_double_zero' rtx
2020-12-16 12:28 ` Richard Sandiford
@ 2020-12-16 13:30 ` Maciej W. Rozycki
2021-01-05 23:51 ` Maciej W. Rozycki
0 siblings, 1 reply; 10+ messages in thread
From: Maciej W. Rozycki @ 2020-12-16 13:30 UTC (permalink / raw)
To: Richard Sandiford; +Cc: Jeff Law, Paul Koning, gcc-patches
On Wed, 16 Dec 2020, Richard Sandiford wrote:
> >> Presumably you can't use CONST0_RTX (mode) here? Assuming that's the
> >> case, then this is fine.
> >
> > Well, `mode' is VOID for simplicity and to match what the middle end
> > presents as an operand to the COMPARE operation, as also shown with the
> > diff above, so with CONST0_RTX (mode) we'd be back to what amounts to
> > `const0_rtx' aka `const_int 0', which is exactly what we want to get away
> > from.
> >
> > Alternatively we could possibly give `const_double_zero' a proper FP
> > mode, but it seems to me like an unnecessary complication, as it would
> > then have to be individually requested and iterated over all the relevant
> > modes.
>
> Generated FP const_double rtxes have to have a proper (non-VOID) mode
> though. VOIDmode CONST_DOUBLEs are always (legacy) integers instead
> of FP values.
>
> So yeah, giving const_double_zero a proper mode seems like the way to go.
> It's technically possible to drop it in a pure matching context, but I'm
> not sure how valuable that is in practice, given that other parts of the
> matched pattern would usually need to refer to the same mode anyway.
Fair enough. For some reason however VOIDmode CONST_DOUBLEs seem to work
with the FP operations in the VAX backend for the purpose of post-reload
comparison elimination while CONST_INTs do not.
> > Have I missed anything here?
> >
> > NB the PDP-11 pieces affected here and tripping this assertion are I
> > believe dead code, as these insns are only ever produced by splitters and
> > do not appear to be referred by their names. We need this change however
> > for completeness so that `const_double_zero' can be used in contexts where
> > an insn actually will be referred by its name.
> >
> > However the PDP-11 code being dead makes it a bit more difficult to
> > examine actual consequences of such a change like I have proposed than it
> > would otherwise be. In these circumstances I think my proposal is safe if
> > a bit overly cautious.
>
> CONST_DOUBLE_ATOF ("0", VOIDmode) seems malformed though, and I'd expect
> it to assert in REAL_MODE_FORMAT (via the format_helper constructor).
> I'm not sure the patch is strictly safer than the status quo.
I may have missed that, though I did follow the chain of calls involved
here to see if there is anything problematic. As I say I have a limited
way to verify this in practice as the PDP-11 code involved appears to me
to be dead, and the situation does not apply to the VAX backend. Maybe I
could simulate it somehow artificially to see what happens.
> FWIW, I agree with Jeff that this ought to be CONST0_RTX (mode).
I'll have to update several places then and push the changes through full
regression testing, so it'll probably take until the next week.
Thanks for your input!
Maciej
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] genemit: Handle `const_double_zero' rtx
2020-12-16 11:35 ` Maciej W. Rozycki
2020-12-16 12:28 ` Richard Sandiford
@ 2020-12-16 19:19 ` Paul Koning
2020-12-16 20:43 ` Maciej W. Rozycki
1 sibling, 1 reply; 10+ messages in thread
From: Paul Koning @ 2020-12-16 19:19 UTC (permalink / raw)
To: Maciej W. Rozycki; +Cc: Jeff Law, GCC Patches
> On Dec 16, 2020, at 6:35 AM, Maciej W. Rozycki <macro@linux-mips.org> wrote:
>
> ...
> NB the PDP-11 pieces affected here and tripping this assertion are I
> believe dead code, as these insns are only ever produced by splitters and
> do not appear to be referred by their names.
Right; I gave them names for documentation purposes, after all insns are allowed to have names whether or not the name is referenced or is a required name for RTL generation.
paul
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] genemit: Handle `const_double_zero' rtx
2020-12-16 19:19 ` Paul Koning
@ 2020-12-16 20:43 ` Maciej W. Rozycki
0 siblings, 0 replies; 10+ messages in thread
From: Maciej W. Rozycki @ 2020-12-16 20:43 UTC (permalink / raw)
To: Paul Koning; +Cc: Jeff Law, GCC Patches
On Wed, 16 Dec 2020, Paul Koning wrote:
> > NB the PDP-11 pieces affected here and tripping this assertion are I
> > believe dead code, as these insns are only ever produced by splitters and
> > do not appear to be referred by their names.
>
> Right; I gave them names for documentation purposes, after all insns are
> allowed to have names whether or not the name is referenced or is a
> required name for RTL generation.
You can use `*foo' however to have it printed in dumps and be able to
refer to it otherwise, while not producing callable `gen_foo' entrypoints.
It is not a feature GCC has always had, but it's been around for a little
while already.
Maciej
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] genemit: Handle `const_double_zero' rtx
2020-12-16 13:30 ` Maciej W. Rozycki
@ 2021-01-05 23:51 ` Maciej W. Rozycki
2021-01-06 16:34 ` Richard Sandiford
0 siblings, 1 reply; 10+ messages in thread
From: Maciej W. Rozycki @ 2021-01-05 23:51 UTC (permalink / raw)
To: Richard Sandiford, Jeff Law; +Cc: Paul Koning, gcc-patches
On Wed, 16 Dec 2020, Maciej W. Rozycki wrote:
> > CONST_DOUBLE_ATOF ("0", VOIDmode) seems malformed though, and I'd expect
> > it to assert in REAL_MODE_FORMAT (via the format_helper constructor).
> > I'm not sure the patch is strictly safer than the status quo.
>
> I may have missed that, though I did follow the chain of calls involved
> here to see if there is anything problematic. As I say I have a limited
> way to verify this in practice as the PDP-11 code involved appears to me
> to be dead, and the situation does not apply to the VAX backend. Maybe I
> could simulate it somehow artificially to see what happens.
I have made an experiment and arranged for a couple of builtins to refer
to CONST_DOUBLE_ATOF ("0", VOIDmode) via expanders and it works just fine
except for failing to match an RTL insn, like:
builtin.c: In function 't':
builtin.c:18:1: error: unrecognizable insn:
18 | }
| ^
(insn 6 3 7 2 (set (reg/v:SF 23 [ f ])
(plus:SF (const_double 0 [0] 0 [0] 0 [0] 0 [0])
(reg/v:SF 32 [ f ]))) "builtin.c":5:6 -1
(nil))
during RTL pass: vregs
builtin.c:18:1: internal compiler error: in extract_insn, at recog.c:2315
so it does work in principle and would produce something if there was a
matching insn.
> > FWIW, I agree with Jeff that this ought to be CONST0_RTX (mode).
>
> I'll have to update several places then and push the changes through full
> regression testing, so it'll probably take until the next week.
FWIW, CONST_DOUBLE_ATOF ("0", VOIDmode) is of course not equivalent to
CONST0_RTX (VOIDmode), as the latter produces a CONST_INT rather than a
CONST_DOUBLE rtx:
builtin.c: In function 't':
builtin.c:18:1: error: unrecognizable insn:
18 | }
| ^
(insn 6 3 7 2 (set (reg/v:SF 23 [ f ])
(plus:SF (const_int 0 [0])
(reg/v:SF 32 [ f ]))) "builtin1.c":5:6 -1
(nil))
during RTL pass: vregs
builtin.c:18:1: internal compiler error: in extract_insn, at recog.c:2315
I suppose we do not have to support VOIDmode here, but I feel a bit uneasy
about the lack of identity mapping between machine description (where in
principle we could already use `const_double' with any mode, not only ones
CONST0_RTX expands to a CONST_DOUBLE for) and RTL produced if CONST0_RTX
was used rather than CONST_DOUBLE_ATOF, as CONST0_RTX does not always
return a CONST_DOUBLE rtx.
For the sake of the experiment I modified machine description further so
as to actually let the rtx produced by CONST_DOUBLE_ATOF ("0", VOIDmode)
through by providing suitable insns, and here's an excerpt from annotated
artificial assembly produced:
#(insn 6 21 7 (set (reg/v:SF 0 %r0 [orig:23 f ] [23])
# (plus:SF (const_double 0 [0] 0 [0] 0 [0] 0 [0])
# (mem/c:SF (plus:SI (reg/f:SI 12 %ap)
# (const_int 4 [0x4])) [1 f+0 S4 A32]))) "builtin.c":5:6 199 {*addzsf3}
# (expr_list:REG_DEAD (reg/f:SI 12 %ap)
# (nil)))
#addf3 $0,4(%ap),%r0 # 6 [c=40] *addzsf3
The CONST_DOUBLE rtx yielded the `$0' operand, so the expression made it
through the backend down to generated assembly (the leading comment
character comes from the artificial `*addzsf3' insn in modified MD).
So with the CONST_DOUBLE_ATOF approach we have a coherent generic model
where we can express arbitrary modes with CONST_DOUBLE rtxes without the
need to analyse in the parser (genemit.c) whether the expression requested
makes sense or not. Whereas with the CONST0_RTX approach we'll either
have to diagnose odd `const_double_zero' usage (making it inconsistent
with `const_zero') or leave it to the undefined (and again inconsistent).
What I think we want to do though is to make CONST_DOUBLE_ATOF ("0",
mode) effectively alias to CONST0_RTX (mode) in the cases where the rtx
produced is of the CONST_DOUBLE type. By the look of `init_emit_once' I
infer we do that already, so I must conclude that the choice between:
printf ("CONST_DOUBLE_ATOF (\"0\", %smode)",
GET_MODE_NAME (GET_MODE (x)));
and:
printf ("CONST0_RTX (%smode)",
GET_MODE_NAME (GET_MODE (x)));
for genemit.c is for those cases merely syntactic. So I think I made the
correct choice here and I'd still rather go with CONST_DOUBLE_ATOF, in
which case we can simply ignore uninteresting modes.
Have I expressed myself clearly enough? I can post the patch I made the
experiments with and builtin.c for the context.
NB the festive season and the turn of events beforehand has delayed me a
bit, but I now have a proper fix for the issue considered here, which
actually removes the current use of VOIDmode with CONST_DOUBLE, and it's
now only a matter of CONST0_RTX vs CONST_DOUBLE_ATOF to be used there.
I'll post the patch shortly and we can continue the discussion in that
context then.
Thank you both for your input.
Maciej
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] genemit: Handle `const_double_zero' rtx
2021-01-05 23:51 ` Maciej W. Rozycki
@ 2021-01-06 16:34 ` Richard Sandiford
2021-01-08 2:12 ` Maciej W. Rozycki
0 siblings, 1 reply; 10+ messages in thread
From: Richard Sandiford @ 2021-01-06 16:34 UTC (permalink / raw)
To: Maciej W. Rozycki; +Cc: Jeff Law, Paul Koning, gcc-patches
"Maciej W. Rozycki" <macro@linux-mips.org> writes:
> On Wed, 16 Dec 2020, Maciej W. Rozycki wrote:
>
>> > CONST_DOUBLE_ATOF ("0", VOIDmode) seems malformed though, and I'd expect
>> > it to assert in REAL_MODE_FORMAT (via the format_helper constructor).
>> > I'm not sure the patch is strictly safer than the status quo.
>>
>> I may have missed that, though I did follow the chain of calls involved
>> here to see if there is anything problematic. As I say I have a limited
>> way to verify this in practice as the PDP-11 code involved appears to me
>> to be dead, and the situation does not apply to the VAX backend. Maybe I
>> could simulate it somehow artificially to see what happens.
>
> I have made an experiment and arranged for a couple of builtins to refer
> to CONST_DOUBLE_ATOF ("0", VOIDmode) via expanders and it works just fine
> except for failing to match an RTL insn, like:
>
> builtin.c: In function 't':
> builtin.c:18:1: error: unrecognizable insn:
> 18 | }
> | ^
> (insn 6 3 7 2 (set (reg/v:SF 23 [ f ])
> (plus:SF (const_double 0 [0] 0 [0] 0 [0] 0 [0])
> (reg/v:SF 32 [ f ]))) "builtin.c":5:6 -1
> (nil))
> during RTL pass: vregs
> builtin.c:18:1: internal compiler error: in extract_insn, at recog.c:2315
>
> so it does work in principle and would produce something if there was a
> matching insn.
Ah, yeah, I'd missed that there was a special case for VOIDmode
in format_helper::format_helper. Still…
>> > FWIW, I agree with Jeff that this ought to be CONST0_RTX (mode).
>>
>> I'll have to update several places then and push the changes through full
>> regression testing, so it'll probably take until the next week.
>
> FWIW, CONST_DOUBLE_ATOF ("0", VOIDmode) is of course not equivalent to
> CONST0_RTX (VOIDmode), as the latter produces a CONST_INT rather than a
> CONST_DOUBLE rtx:
>
> builtin.c: In function 't':
> builtin.c:18:1: error: unrecognizable insn:
> 18 | }
> | ^
> (insn 6 3 7 2 (set (reg/v:SF 23 [ f ])
> (plus:SF (const_int 0 [0])
> (reg/v:SF 32 [ f ]))) "builtin1.c":5:6 -1
> (nil))
> during RTL pass: vregs
> builtin.c:18:1: internal compiler error: in extract_insn, at recog.c:2315
>
> I suppose we do not have to support VOIDmode here, but I feel a bit uneasy
> about the lack of identity mapping between machine description (where in
> principle we could already use `const_double' with any mode, not only ones
> CONST0_RTX expands to a CONST_DOUBLE for) and RTL produced if CONST0_RTX
> was used rather than CONST_DOUBLE_ATOF, as CONST0_RTX does not always
> return a CONST_DOUBLE rtx.
Both of the insns above are malformed though. Floating-point
constants have to have floating-point modes. const_ints and
VOIDmode const_doubles are always integers instead.
So even if CONST_DOUBLE_ATOF (…, VOIDmode) fails to ICE, I think it's
a constraint violation in that it's using a floating-point routine to
construct an integer rtx representation.
VOIDmode const_doubles should only be used for integers that cannot
be expressed as a sign-extended HOST_WIDE_INT. So (const_double 0 0)
is an invalid rtx in both integer and FP contexts.
> For the sake of the experiment I modified machine description further so
> as to actually let the rtx produced by CONST_DOUBLE_ATOF ("0", VOIDmode)
> through by providing suitable insns, and here's an excerpt from annotated
> artificial assembly produced:
>
> #(insn 6 21 7 (set (reg/v:SF 0 %r0 [orig:23 f ] [23])
> # (plus:SF (const_double 0 [0] 0 [0] 0 [0] 0 [0])
> # (mem/c:SF (plus:SI (reg/f:SI 12 %ap)
> # (const_int 4 [0x4])) [1 f+0 S4 A32]))) "builtin.c":5:6 199 {*addzsf3}
> # (expr_list:REG_DEAD (reg/f:SI 12 %ap)
> # (nil)))
> #addf3 $0,4(%ap),%r0 # 6 [c=40] *addzsf3
>
> The CONST_DOUBLE rtx yielded the `$0' operand, so the expression made it
> through the backend down to generated assembly (the leading comment
> character comes from the artificial `*addzsf3' insn in modified MD).
Yeah, unfortunately RTL lacks the sanitary checks that gimple has,
so it's not too surprising that the pattern makes it through to final.
It's still malformed though.
(FTR, the constant should also be the second operand to the plus,
but that's obviously tangential.)
Thanks,
Richard
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] genemit: Handle `const_double_zero' rtx
2021-01-06 16:34 ` Richard Sandiford
@ 2021-01-08 2:12 ` Maciej W. Rozycki
0 siblings, 0 replies; 10+ messages in thread
From: Maciej W. Rozycki @ 2021-01-08 2:12 UTC (permalink / raw)
To: Richard Sandiford; +Cc: Jeff Law, Paul Koning, gcc-patches
On Wed, 6 Jan 2021, Richard Sandiford wrote:
> VOIDmode const_doubles should only be used for integers that cannot
> be expressed as a sign-extended HOST_WIDE_INT. So (const_double 0 0)
> is an invalid rtx in both integer and FP contexts.
The updated change makes the VAX and PDP-11 backends stop producing
those, but as I noted this is IMHO tangential to the choice between
CONST0_RTX and CONST_DOUBLE_ATOF for code generators produced from named
insns. And the latter does not interpret the mode, i.e. does not enforce
the policy (which itself I don't argue against), leaving it up to people
writing machine descriptions to get it right, which I think is the right
place.
> (FTR, the constant should also be the second operand to the plus,
> but that's obviously tangential.)
Also FTR, I made it the first one deliberately, as I made my experimental
change across all the four basic arithmetic operations, so for obvious
reasons I wanted to prevent a divisor from being 0 lest the middle end get
anxious about it. Also contrary to documentation the middle end does
present constants as the first operand in some cases where it is not
supposed to (and does insist on doing so even if the backend says it's
more expensive), as it was previously discussed in the context of COMPARE.
Actually the VAX machine operand syntax is generic enough you can have an
immediate as any operand, and in particular the first input to binary
operations -- e.g. the minuend, and obviously also the first addend -- in
their three-operand encodings; all the four basic arithmetic operations
have them.
I have posted the updated change as a patch series now, including you in
the list of the recipients in case you wanted to chime in. I appreciate
your feedback and experience in this area, thank you!
Maciej
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-01-08 2:13 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-15 20:38 [PATCH] genemit: Handle `const_double_zero' rtx Maciej W. Rozycki
2020-12-15 23:20 ` Jeff Law
2020-12-16 11:35 ` Maciej W. Rozycki
2020-12-16 12:28 ` Richard Sandiford
2020-12-16 13:30 ` Maciej W. Rozycki
2021-01-05 23:51 ` Maciej W. Rozycki
2021-01-06 16:34 ` Richard Sandiford
2021-01-08 2:12 ` Maciej W. Rozycki
2020-12-16 19:19 ` Paul Koning
2020-12-16 20:43 ` Maciej W. Rozycki
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).