[Bug target/102211] New: ICE introduced by r12-3277

[Bug target/102211] New: ICE introduced by r12-3277

[crazylht at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102211

            Bug ID: 102211
           Summary: ICE introduced by r12-3277
           Product: gcc
           Version: 12.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: crazylht at gmail dot com
                CC: schwab@linux-m68k.org
  Target Milestone: ---
            Target: risv64

cat test.c

float
foo (float a, long b)
{
  union{float a[2];
    long b;}c;
  c.b = b;
  return c.a[0] * a;
}


gcc test.c -O2 -S

test.c:8:1: error: this is the insn:
(insn 12 19 13 2 (set (reg/i:SF 42 fa0)
        (mult:SF (reg:SF 77)
            (subreg:SF (reg:DI 80 [78]) 0))) "test.c":8:1 17 {mulsf3}
     (expr_list:REG_DEAD (reg:DI 80 [78])
        (expr_list:REG_DEAD (reg:SF 77)
            (nil))))
during RTL pass: reload

[Bug target/102211] ICE introduced by r12-3277

[crazylht at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102211

--- Comment #1 from Hongtao.liu <crazylht at gmail dot com> ---
But it's ok for

float
foo (float a, long b)
{
  union{float a[2];
    long b;}c;
  c.b = b;
  return c.a[0];
}


foo:
        fmv.w.x fa0,a0
        ret

Which means movement between gpr and float reg is allowed.

[Bug target/102211] ICE introduced by r12-3277

[crazylht at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102211

--- Comment #2 from Hongtao.liu <crazylht at gmail dot com> ---
According to *movsi_internal and *movdi_64bit, SImode, and DImode can be placed
into FP_REGS, but in riscv_hard_regno_mode_ok, SImode/DImode is not allowed to
be allocated as FP_REGS, the mismatch here caues the ICE.

Simple hack as

modified   gcc/config/riscv/riscv.c
@@ -4553,7 +4553,9 @@ riscv_hard_regno_mode_ok (unsigned int regno,
machine_mode mode)
        return false;

       if (GET_MODE_CLASS (mode) != MODE_FLOAT
-         && GET_MODE_CLASS (mode) != MODE_COMPLEX_FLOAT)
+         && GET_MODE_CLASS (mode) != MODE_COMPLEX_FLOAT
+         && mode != E_SImode
+         && mode != E_DImode)
        return false;

       /* Only use callee-saved registers if a potential callee is guaranteed

Solved the issue.

foo:
        fmv.d.x fa5,a0
        fmul.s  fa0,fa0,fa5

[Bug target/102211] [12 regression] ICE introduced by r12-3277

[schwab@linux-m68k.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102211

Andreas Schwab <schwab@linux-m68k.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|ICE introduced by r12-3277  |[12 regression] ICE
                   |                            |introduced by r12-3277
           Keywords|                            |build

--- Comment #3 from Andreas Schwab <schwab@linux-m68k.org> ---
This also breaks bootstrap.

[Bug target/102211] [12 regression] ICE introduced by r12-3277

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102211

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |12.0

[Bug target/102211] [12 regression] ICE introduced by r12-3277

[wilson at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102211

Jim Wilson <wilson at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |wilson at gcc dot gnu.org

--- Comment #4 from Jim Wilson <wilson at gcc dot gnu.org> ---
Yes, moving SI/DI values to FP regs is OK.  However, RISC-V requires that FP
values in FP registers be stored NaN-boxed.  So an SFmode value in a 64-bit FP
reg has the upper 32-bits of all ones, and the lower 32-bits is the value. 
Thus if accessed as a 64-bit value, you get a NaN.  The hardware may trap if
you access a 32-bit value which is not properly NaN-boxed.  Using qemu to check
this may not be good enough, as last time I looked at qemu it wasn't handling
NaN-boxing correctly, but this was over a year ago, so maybe it has been fixed
since.  I don't know.

Anyways, this code sequence is OK
foo:
        fmv.w.x fa0,a0
        ret
because we are moving a 32-bit SImode value to an FP reg and then treating it
as SFmode, and the 32-bit move will properly NaN-box the SFmode value.

This code sequence is not OK
foo:
        fmv.d.x fa5,a0
        fmul.s  fa0,fa0,fa5
because we are moving a 64-bit DImode value to an FP reg and then treating it
as SFmode, which is not OK because the value won't be NaN-boxed and may trap at
run time.

validate_subreg used to prevent the bad subreg from being created.

I would think that TARGET_CAN_CHANGE_MODE_CLASS could help here, but it isn't
being called inside general_operand when called from fwprop1 where the bad
substitution happens.  Because we have a pseudo-register, and it is only called
for hard registers.

I don't see a way to fix this as a backend change with current validate_subreg,
other than by replacing register_operand with riscv_register_operand, and
putting the subreg check I need inside riscv_register_operand.  And likewise
for any other affected predicate, like move_operand.  This will be a big
change, though a lot of it will be mechanical.  As an optimization, we can
continue to use register_operand in any pattern that can't use FP registers.

As a middle end change, I need a new hook in general_operand to reject subregs
that we can't support on RISC-V.

Or maybe re-add the check I need to validate_subreg as a hook, so it can be
conditionally enabled for RISC-V.

We can allow (subreg:SF (reg:DI)) if it gets allocated to an integer register. 
It is only when it is allocated to an FP register that it can't work.  I don't
know offhand if that can be described.  But disallowing the subreg always for
RISC-V is simpler and also works.

Re: [Bug target/102211] [12 regression] ICE introduced by r12-3277

[Andrew Waterman]

On Tue, Sep 7, 2021 at 10:55 PM wilson at gcc dot gnu.org via Gcc-bugs
<gcc-bugs@gcc.gnu.org> wrote:
>
> The hardware may trap if
> you access a 32-bit value which is not properly NaN-boxed.

I don't think the following affects the resolution of the ticket, but
just for the record, trapping is _not_ an option the ISA permits.  The
only legal outcome from using an improperly NaN-boxed value as an
argument to an instruction that requires NaN-boxed inputs is that the
canonical NaN is substituted in its place.

[Bug target/102211] [12 regression] ICE introduced by r12-3277

[wilson at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102211

Jim Wilson <wilson at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2021-09-08
             Status|UNCONFIRMED                 |NEW

[Bug target/102211] [12 regression] ICE introduced by r12-3277

[wilson at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102211

--- Comment #5 from Jim Wilson <wilson at gcc dot gnu.org> ---
I have a WIP fix that lets me build newlib and glibc via riscv-gnu-toolchain. 
I haven't tried a bootstrap yet.  I created a new predicate that uses the small
bit of deleted code I need from validate_subregs, and then modified most
patterns with an f constraint to use it.

+;; This predicate should be used for any pattern that has constraints that
+;; accept FP registers.
+(define_predicate "f_register_operand"
+  (match_operand 0 "register_operand")
+{
+  /* We can't allow a subreg that changes size in an FP reg, as that violates
+     the NaN-boxing requirement.  Copied from old validate_subregs code.  */
+  if (GET_CODE (op) == SUBREG)
+    {
+      machine_mode imode = GET_MODE (SUBREG_REG (op));
+      machine_mode omode = GET_MODE (op);
+      if (FLOAT_MODE_P (imode) || FLOAT_MODE_P (omode))
+       {
+         poly_uint64 isize = GET_MODE_SIZE (imode);
+         poly_uint64 osize = GET_MODE_SIZE (omode);
+
+         if (! (known_eq (isize, osize)
+                /* LRA can use subreg to store a floating point value in
+                   an integer mode.  Although the floating point and the
+                   integer modes need the same number of hard registers,
+                   the size of floating point mode can be less than the
+                   integer mode.  LRA also uses subregs for a register
+                   should be used in different mode in on insn.  */
+                || lra_in_progress))
+           return false;
+       }
+    }
+  return true;
+})

I haven't fixed the move patterns yet.  I need a few more new predicates for
that.

The riscv_hard_regno_mode_ok function looks odd as Hongtao Liu mentioned.  The
mov*i patterns have f constraints, but we don't allow FP values in integer
registers here.  I think this is a missed optimization, or maybe good register
allocation creates some no-op moves to hide the problem.  My current patch does
not need a riscv_hard_regno_mode_ok change to work.

The riscv_can_change_mode_class also looks a little odd.  The MIPS one that we
copied doesn't allow mode changes in FP regs, but the alpha one does allow mode
changes in FP regs if the modes have the same size.  I think the alpha one can
work for RISC-V and that this is another missed optimization, though again
maybe good regalloc hides the problem with no-op moves.

But I will worry about the possible missed optimizations after I get bootstrap
working again.

[Bug target/102211] [12 regression] ICE introduced by r12-3277

[segher at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102211

Segher Boessenkool <segher at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |meissner at gcc dot gnu.org,
                   |                            |segher at gcc dot gnu.org

--- Comment #6 from Segher Boessenkool <segher at gcc dot gnu.org> ---
(In reply to Jim Wilson from comment #4)
> Yes, moving SI/DI values to FP regs is OK.  However, RISC-V requires that FP
> values in FP registers be stored NaN-boxed.  So an SFmode value in a 64-bit
> FP reg has the upper 32-bits of all ones, and the lower 32-bits is the
> value.  Thus if accessed as a 64-bit value, you get a NaN.

On Power, a (scalar) SF value is usually stored in DF format.  It is the insns
that force the outputs to SP, the inputs in general can be anything.

> The hardware may
> trap if you access a 32-bit value which is not properly NaN-boxed.

We used to have such games as well :-)  Thankfully it was largely transparent.

> Using
> qemu to check this may not be good enough, as last time I looked at qemu it
> wasn't handling NaN-boxing correctly, but this was over a year ago, so maybe
> it has been fixed since.  I don't know.

QEMU in general optimises for speed, not for correct emulation.  If you have
inputs that are invalid it will have more surprising outputs.

> This code sequence is not OK
> foo:
> 	fmv.d.x	fa5,a0
> 	fmul.s	fa0,fa0,fa5
> because we are moving a 64-bit DImode value to an FP reg and then treating
> it as SFmode, which is not OK because the value won't be NaN-boxed and may
> trap at run time.

A situation very similar to the Power problem.

> I would think that TARGET_CAN_CHANGE_MODE_CLASS could help here, but it
> isn't being called inside general_operand when called from fwprop1 where the
> bad substitution happens.  Because we have a pseudo-register, and it is only
> called for hard registers.

The documentation says

===
@cindex @code{TARGET_CAN_CHANGE_MODE_CLASS} and subreg semantics
The rules above apply to both pseudo @var{reg}s and hard @var{reg}s.
If the semantics are not correct for particular combinations of
@var{m1}, @var{m2} and hard @var{reg}, the target-specific code
must ensure that those combinations are never used.  For example:

@smallexample
TARGET_CAN_CHANGE_MODE_CLASS (@var{m2}, @var{m1}, @var{class})
@end smallexample

must be false for every class @var{class} that includes @var{reg}.
===

so the code does not do what the documentation says?

> I don't see a way to fix this as a backend change with current
> validate_subreg, other than by replacing register_operand with
> riscv_register_operand, and putting the subreg check I need inside
> riscv_register_operand.  And likewise for any other affected predicate, like
> move_operand.  This will be a big change, though a lot of it will be
> mechanical.  As an optimization, we can continue to use register_operand in
> any pattern that can't use FP registers.

The first thing that will have to be done is to restore the status quo, to
make your, my, and all other affected targets work again (there very likely
are more, the problems are all in not-so-very normal cases, not to mention
not all targets are tested so heavily -- which reinforces my argument that
there should have been testsuite cases added that trap this on all targets).

After that, yeah, our backends should be improved.  That requires some new
stuff in the middle end as well afaics, there really are reasons Power and
RISC-V both did such terrible thing -- but it certainly should not be done
with a knife at the throat, this is some serious re-engineering, it cannot
be done at a snap of the fingers.

> As a middle end change, I need a new hook in general_operand to reject
> subregs that we can't support on RISC-V.

That may be a good design that is suitable for others as well, it is quite
nicely general.  Mike, will that do all we need for the SF subregs as well?

A little problem with this is most of our operands do not inherit from
general_operand.  A somewhat bigger problem is: what about predicates, do
those not need changes as well, for good machine code quality?

> Or maybe re-add the check I need to validate_subreg as a hook, so it can be
> conditionally enabled for RISC-V.

Yeah...  But it should be re-enabled *by default*, to start with.

> We can allow (subreg:SF (reg:DI)) if it gets allocated to an integer
> register.  It is only when it is allocated to an FP register that it can't
> work.  I don't know offhand if that can be described.  But disallowing the
> subreg always for RISC-V is simpler and also works.

(subreg:SF (reg:DI)) does two things at once: an actual subreg, and a bit_cast.
We should not express both of those with the same rtx code, since we do not
allow subregs of subregs (and that is a good thing!)

[Bug target/102211] [12 regression] ICE introduced by r12-3277

[andrew at sifive dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102211

--- Comment #7 from Andrew Waterman <andrew at sifive dot com> ---
On Tue, Sep 7, 2021 at 10:55 PM wilson at gcc dot gnu.org via Gcc-bugs
<gcc-bugs@gcc.gnu.org> wrote:
>
> The hardware may trap if
> you access a 32-bit value which is not properly NaN-boxed.

I don't think the following affects the resolution of the ticket, but
just for the record, trapping is _not_ an option the ISA permits.  The
only legal outcome from using an improperly NaN-boxed value as an
argument to an instruction that requires NaN-boxed inputs is that the
canonical NaN is substituted in its place.

[Bug target/102211] [12 regression] ICE introduced by r12-3277

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102211

--- Comment #8 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by hongtao Liu <liuhongt@gcc.gnu.org>:

https://gcc.gnu.org/g:57b7c432cce893e1ba60d9b94a9606df6b419379

commit r12-3457-g57b7c432cce893e1ba60d9b94a9606df6b419379
Author: liuhongt <hongtao.liu@intel.com>
Date:   Fri Sep 10 20:02:25 2021 +0800

    Revert "Get rid of all float-int special cases in validate_subreg."

    This reverts commit d2874d905647a1d146dafa60199d440e837adc4d.

    PR target/102254
    PR target/102154
    PR target/102211

[Bug target/102211] [12 regression] ICE introduced by r12-3277

[wilson at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102211

--- Comment #9 from Jim Wilson <wilson at gcc dot gnu.org> ---
Created attachment 51456
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=51456&action=edit
proposed RISC-V backend solution

[Bug target/102211] [12 regression] ICE introduced by r12-3277

[wilson at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102211

--- Comment #10 from Jim Wilson <wilson at gcc dot gnu.org> ---
I attached a patch which is my proposed solution to the RISC-V backend.  It
adds a new f_register_operand predicate and modifies patterns that use the f
constraint to use it instead of register_operand.

This was tested with an default language gcc build, glibc build, and glibc
check on an unmatched running OpenEmbedded.  And an all language gcc build,
glibc build, and glibc check on an unleashed running Fedora with an old 4.15
kernel.  Both succeeded as well as expected.  I'll be trying gcc check next.

Meanwhile, the validate_subregs patch was reverted, so we don't immediately
need this to fix build errors.  But it still might be useful if
validate_subregs changes again.  Or if it happens to give better code, though I
think it won't do anything if validate_subregs is rejecting the subregs we are
checking for in f_register_operand.

[Bug target/102211] [12 regression] ICE introduced by r12-3277

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102211

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|NEW                         |RESOLVED

--- Comment #11 from Richard Biener <rguenth at gcc dot gnu.org> ---
Fixed since reverted.

[Bug target/102211] [12 regression] ICE introduced by r12-3277

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102211
Bug 102211 depends on bug 102154, which changed state.

Bug 102154 Summary: [12 Regression] ICE in extract_insn, at recog.c:2769 since r12-3277-gd2874d905647a1d146dafa60199d440e837adc4d
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102154

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED