[PATCH][gcse] Use emit_move_insn rather than creating SET rtx and emitting that

[PATCH][gcse] Use emit_move_insn rather than creating SET rtx and emitting that

[Kyrill Tkachov]

[-- Attachment #1: Type: text/plain, Size: 1192 bytes --]

Hi all,

This patches fixes the ICE reported at 
https://gcc.gnu.org/ml/gcc-patches/2015-03/msg00949.html
The problem is that gcse generates a (set (reg:OI) (const_int 0)) that 
doesn't match anything
in the arm backend. That SET was created through processing an insn 
generated by the neon_movoi
insn in neon.md. gcse created an OImode reg, put it in a SET rtx with 
const_int 0 and tried to
recognise it which failed.

As pointed out by James Greenhalgh offline the correct thing would have 
been to do an
emit_move_insn to let the backend expanders do the right thing 
(especially in the concerned
testcase gcc.c-torture/execute/pr65427.c that uses 256-bit vectors that 
arm doesn't support
natively).

Bootstrapped and tested on arm, x86, aarch64.
This ICE doesn't happen with 4.9 and 4.8 so it's only a regression for 
GCC 5.
The currently ICE'ins testcase passes now, so no new testcase is added.

Ok for trunk?

Thanks,
Kyrill

2015-03-18  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
             James Greenhalgh  <james.greenhalgh@arm.com>

     * gcse.c (process_insert_insn): Use emit_move_insn rather than
     generating a SET rtx and emitting it.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: gcse.patch --]
[-- Type: text/x-patch; name=gcse.patch, Size: 672 bytes --]

commit 9b6366d27deb2a366c7cfd308e02ab158527f430
Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com>
Date:   Wed Mar 18 16:05:36 2015 +0000

    [gcse] Use emit_move_insn rather than creating SET rtx and emitting that

diff --git a/gcc/gcse.c b/gcc/gcse.c
index e03b36c..cc55e91 100644
--- a/gcc/gcse.c
+++ b/gcc/gcse.c
@@ -2168,7 +2168,7 @@ process_insert_insn (struct gcse_expr *expr)
      insn will be recognized (this also adds any needed CLOBBERs).  */
   else
     {
-      rtx_insn *insn = emit_insn (gen_rtx_SET (VOIDmode, reg, exp));
+      rtx_insn *insn = emit_move_insn (reg, exp);
 
       if (insn_invalid_p (insn, false))
 	gcc_unreachable ();

Re: [PATCH][gcse] Use emit_move_insn rather than creating SET rtx and emitting that

[Jakub Jelinek]

On Thu, Mar 19, 2015 at 01:45:19PM +0000, Kyrill Tkachov wrote:
> Bootstrapped and tested on arm, x86, aarch64.
> This ICE doesn't happen with 4.9 and 4.8 so it's only a regression for GCC
> 5.
> The currently ICE'ins testcase passes now, so no new testcase is added.

Not an expert on this, but it looks wrong to me.
emit_move_insn is used a few lines above, but only for the general_operand
case, so it seems it was very much intentional that way.  I bet
emit_move_insn isn't prepared to handle arbitrary RTL expressions, so the
general_operand check makes sense for it.
As process_insert_insn supposed can't fail, perhaps something earlier should
have figured out it would be invalid?

	Jakub

Re: [PATCH][gcse] Use emit_move_insn rather than creating SET rtx and emitting that

[Steven Bosscher]

On Thu, Mar 19, 2015 at 2:45 PM, Kyrill Tkachov wrote:
> As pointed out by James Greenhalgh offline the correct thing would have been
> to do an
> emit_move_insn to let the backend expanders do the right thing (especially
> in the concerned
> testcase gcc.c-torture/execute/pr65427.c that uses 256-bit vectors that arm
> doesn't support
> natively).

This is supposed to be caught by want_to_gcse_p() via
can_assign_to_reg_without_clobbers_p(). How does your expression get
past that barrier?

The gcc_unreachable() is there because all the code in gcse.c assumes
it is OK to emit a SET-insn without going through emit_move_insn().

Ciao!
Steven

Re: [PATCH][gcse] Use emit_move_insn rather than creating SET rtx and emitting that

[Kyrill Tkachov]

On 19/03/15 13:56, Steven Bosscher wrote:
> On Thu, Mar 19, 2015 at 2:45 PM, Kyrill Tkachov wrote:
>> As pointed out by James Greenhalgh offline the correct thing would have been
>> to do an
>> emit_move_insn to let the backend expanders do the right thing (especially
>> in the concerned
>> testcase gcc.c-torture/execute/pr65427.c that uses 256-bit vectors that arm
>> doesn't support
>> natively).
> This is supposed to be caught by want_to_gcse_p() via
> can_assign_to_reg_without_clobbers_p(). How does your expression get
> past that barrier?
>
> The gcc_unreachable() is there because all the code in gcse.c assumes
> it is OK to emit a SET-insn without going through emit_move_insn().

Thanks for the pointers, I was too hasty with that patch.
I'm not very familiar with the gcse code so I'll dig around.

Kyrill

> Ciao!
> Steven
>

Re: [PATCH][gcse] Use emit_move_insn rather than creating SET rtx and emitting that

[Kyrill Tkachov]

On 19/03/15 13:56, Steven Bosscher wrote:
> On Thu, Mar 19, 2015 at 2:45 PM, Kyrill Tkachov wrote:
>> As pointed out by James Greenhalgh offline the correct thing would have been
>> to do an
>> emit_move_insn to let the backend expanders do the right thing (especially
>> in the concerned
>> testcase gcc.c-torture/execute/pr65427.c that uses 256-bit vectors that arm
>> doesn't support
>> natively).
> This is supposed to be caught by want_to_gcse_p() via
> can_assign_to_reg_without_clobbers_p(). How does your expression get
> past that barrier?
>
> The gcc_unreachable() is there because all the code in gcse.c assumes
> it is OK to emit a SET-insn without going through emit_move_insn().

btw, the ICE happens during the hoist pass, rather than gcse.
Kyrill

> Ciao!
> Steven
>