[Bug target/62014] New: [AArch64] Using -mgeneral-regs-only may lead to ICE

[Bug target/62014] New: [AArch64] Using -mgeneral-regs-only may lead to ICE

[e.menezes at samsung dot com]

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

            Bug ID: 62014
           Summary: [AArch64] Using -mgeneral-regs-only may lead to ICE
           Product: gcc
           Version: 4.10.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: e.menezes at samsung dot com

Created attachment 33245
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=33245&action=edit
This patch should fix this issue, though it needs a test-case.

In some cases, when the LRA spills a register into an FP register, with the
option -mgeneral-regs-only specified, there is an ICE.

It seems to be caused by the LRA assuming that the FP registers are always
available and not being told when they aren't by the target.

[Bug target/62014] [AArch64] Using -mgeneral-regs-only may lead to ICE

[pinskia at gcc dot gnu.org]

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

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
+  /* Do not spill into FP registers when "-mgeneral-regs-only" is specified. *

You are missing a / in your comment.

[Bug target/62014] [AArch64] Using -mgeneral-regs-only may lead to ICE

[e.menezes at samsung dot com]

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

--- Comment #2 from Evandro Menezes <e.menezes at samsung dot com> ---
(In reply to Andrew Pinski from comment #1)
> +  /* Do not spill into FP registers when "-mgeneral-regs-only" is
> specified. *
> 
> You are missing a / in your comment.

Ermahgerd!

[Bug target/62014] [AArch64] Using -mgeneral-regs-only may lead to ICE

[e.menezes at samsung dot com]

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

Evandro Menezes <e.menezes at samsung dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #33245|0                           |1
        is obsolete|                            |

--- Comment #3 from Evandro Menezes <e.menezes at samsung dot com> ---
Created attachment 33246
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=33246&action=edit
This patch should fix this issue, though it needs a test-case

[Bug target/62014] [AArch64] Using -mgeneral-regs-only may lead to ICE

[ktkachov at gcc dot gnu.org]

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

ktkachov at gcc dot gnu.org changed:

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

--- Comment #4 from ktkachov at gcc dot gnu.org ---
(In reply to Evandro Menezes from comment #3)
> Created attachment 33246 [details]
> This patch should fix this issue, though it needs a test-case

Please post to gcc-patches with a testcase.
I'd prefer if you used TARGET_FP rather than !TARGET_GENERAL_REGS_ONLY

[Bug target/62014] [AArch64] Using -mgeneral-regs-only may lead to ICE

[ramana at gcc dot gnu.org]

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

Ramana Radhakrishnan <ramana at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Target|                            |aarch64-*
             Status|UNCONFIRMED                 |WAITING
   Last reconfirmed|                            |2014-08-05
     Ever confirmed|0                           |1

--- Comment #5 from Ramana Radhakrishnan <ramana at gcc dot gnu.org> ---
According to the documentation, not defining TARGET_SPILL_CLASS is equivalent
to returning NO_REGS for all inputs. 

The symptoms that you describe indicate a problem elsewhere or an attempt to
paper over the problem rather than the correct fix. The smoking gun for me is
to suspect an errant pattern with incorrectly marked simd attributes rather
than anything else. Look at the way in which the simd attribute is set on
*addsi3_aarch64 for e.g.

Without a testcase and without instructions to reproduce the problems as per
https://gcc.gnu.org/bugs/#report it is not possible to work out what's going on
here.

Next time please also remember to fill in Target, Known to work, Known to fail
and Keywords fields as well.

[Bug target/62014] [AArch64] Using -mgeneral-regs-only may lead to ICE

[ktkachov at gcc dot gnu.org]

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

--- Comment #6 from ktkachov at gcc dot gnu.org ---
One crude way to look is by looking for the "w" constraint that signifies usage
of FP registers in the md files and seeing whether those usages are properly
guarded by the 'simd' attribute or by TARGET_FLOAT/TARGET_SIMD.

One quick search shows the absdi2 define_insn_and_split that uses 'w' in its
second alternative to emit "abs\\t%d0, %d1" but doesn't guard it by the 'simd'
attribute. Maybe that's a place to start to construct a testcase

[Bug target/62014] [AArch64] Using -mgeneral-regs-only may lead to ICE

[ramana at gcc dot gnu.org]

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

--- Comment #7 from Ramana Radhakrishnan <ramana at gcc dot gnu.org> ---
(In reply to ktkachov from comment #6)
> One crude way to look is by looking for the "w" constraint that signifies
> usage of FP registers in the md files and seeing whether those usages are
> properly guarded by the 'simd' attribute or by TARGET_FLOAT/TARGET_SIMD.
> 
> One quick search shows the absdi2 define_insn_and_split that uses 'w' in its
> second alternative to emit "abs\\t%d0, %d1" but doesn't guard it by the
> 'simd' attribute. Maybe that's a place to start to construct a testcase

Thanks, I was about to do this after my morning cuppa :) 

If this was noticed from a testcase, it should be trivial to work out which
pattern it was that had the w registers by just looking at the output with -dp.
Anyway I agree that absdi2 looks the most suspicious.

[Bug target/62014] [AArch64] Using -mgeneral-regs-only may lead to ICE

[ktkachov at gcc dot gnu.org]

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

--- Comment #8 from ktkachov at gcc dot gnu.org ---
Also, James pointed out the patterns in the region of aarch64_movtilow_tilow.
They use SIMD registers to move wide values but are not guarded.

[Bug target/62014] [AArch64] Using -mgeneral-regs-only may lead to ICE

[e.menezes at samsung dot com]

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

--- Comment #9 from Evandro Menezes <e.menezes at samsung dot com> ---
It seems to me that it's the LRA which is forcing the use of FP registers, so,
even if the patterns are fixed, I believe that in the end the combiner would
just give up and ICE.  With this assumption, which is open to corrections, I
believe that the LRA needs to be properly managed according to the options
passed on to the target.

[Bug target/62014] [AArch64] Using -mgeneral-regs-only may lead to ICE

[ktkachov at gcc dot gnu.org]

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

--- Comment #10 from ktkachov at gcc dot gnu.org ---
What we really need here is a preprocessed testcase showing the problem.
It should be fairly easy to lock down on the problem then

[Bug target/62014] [AArch64] Using -mgeneral-regs-only may lead to ICE

[e.menezes at samsung dot com]

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

--- Comment #11 from Evandro Menezes <e.menezes at samsung dot com> ---
(In reply to ktkachov from comment #10)
> What we really need here is a preprocessed testcase showing the problem.
> It should be fairly easy to lock down on the problem then

I'm on it.

[Bug target/62014] [AArch64] Using -mgeneral-regs-only may lead to ICE

[rearnsha at gcc dot gnu.org]

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

--- Comment #12 from Richard Earnshaw <rearnsha at gcc dot gnu.org> ---
aarch64_conditional_register_usage() marks all FP registers as unavailable if
!TARGET_FLOAT.  So the real question is why this isn't sufficient to disable
use of FP registers.

[Bug target/62014] [AArch64] Using -mgeneral-regs-only may lead to ICE

[e.menezes at samsung dot com]

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

--- Comment #13 from Evandro Menezes <e.menezes at samsung dot com> ---
Created attachment 33253
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=33253&action=edit
Test-case

This test-case is a stripped-down version of Dhrystone, where the issue was
first observed.

Built with just -O2, it ends up with a handful of FMOVs, which are then avoided
if -mno-lra is also specified.

[Bug target/62014] [AArch64] Using -mgeneral-regs-only may lead to ICE

[e.menezes at samsung dot com]

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

Evandro Menezes <e.menezes at samsung dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #33246|0                           |1
        is obsolete|                            |

--- Comment #14 from Evandro Menezes <e.menezes at samsung dot com> ---
Created attachment 33254
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=33254&action=edit
Patch

For the sake of correctness, this patch uses the more generic flag to qualify
the spilling.

[Bug target/62014] [AArch64] Using -mgeneral-regs-only may lead to ICE

[ktkachov at gcc dot gnu.org]

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

--- Comment #15 from ktkachov at gcc dot gnu.org ---
I can't reproduce this with current trunk and 4.9.1.
What exact compiler version and options did you use?
I used -O2 -mgeneral-regs-only on an aarch64-none-elf compiler
gcc version 4.10.0 20140805

[Bug target/62014] [AArch64] Using -mgeneral-regs-only may lead to ICE

[e.menezes at samsung dot com]

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

Evandro Menezes <e.menezes at samsung dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|WAITING                     |RESOLVED
         Resolution|---                         |WORKSFORME

--- Comment #16 from Evandro Menezes <e.menezes at samsung dot com> ---
If it's working, it's good for me.