"Maciej W. Rozycki" <ma...@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
