Patchwork Bfin: Ensure rotrsi and rotlsi don't accept non-const INTVALS

login
register
mail settings
Submitter Bernd Schmidt
Date March 31, 2011, 4:01 p.m.
Message ID <4D94A55D.1020202@codesourcery.com>
Download mbox | patch
Permalink /patch/89092/
State New
Headers show

Comments

Bernd Schmidt - March 31, 2011, 4:01 p.m.
On 03/31/2011 05:42 PM, Richard Henderson wrote:
>>  	(rotate:SI (match_operand:SI 1 "register_operand" "")
>> -		   (match_operand:SI 2 "immediate_operand" "")))]
>> +		   (match_operand:SI 2 "const_int_operand" "")))]
>>    ""
>>  {
>> -  if (INTVAL (operands[2]) != 16)
>> +  if (GET_CODE (operands[2]) != CONST_INT || INTVAL (operands[2]) != 16)
>>      FAIL;
> 
> The point was, that you'd not need the CONST_INT check anymore,
> because it's handled by the predicate.

I have a dim memory of the problem being that something didn't check the
predicate. Sure enough, with the patch below applied to a 4.3 tree, I get

/local/src/egcs/gcc-4_3-branch/gcc/testsuite/gcc.c-torture/execute/20020226-1.c:43:
internal compiler error: in gen_rotrsi3, at config/bfin/bfin.md:1632
Please submit a full bug report,
with preprocessed source if appropriate.
See <http://gcc.gnu.org/bugs.html> for instructions.
compiler exited with status 1
output is:
(reg:SI 193)

Stuart, try to identify whether it still happens. If so, a better fix
would be to change the expanders to honour the predicate.


Bernd
Richard Henderson - March 31, 2011, 5:16 p.m.
On 03/31/2011 09:01 AM, Bernd Schmidt wrote:
> I have a dim memory of the problem being that something didn't check the
> predicate. Sure enough, with the patch below applied to a 4.3 tree, I get
> 
> /local/src/egcs/gcc-4_3-branch/gcc/testsuite/gcc.c-torture/execute/20020226-1.c:43:
> internal compiler error: in gen_rotrsi3, at config/bfin/bfin.md:1632
> Please submit a full bug report,
> with preprocessed source if appropriate.
> See <http://gcc.gnu.org/bugs.html> for instructions.
> compiler exited with status 1
> output is:
> (reg:SI 193)
> 
> Stuart, try to identify whether it still happens. If so, a better fix
> would be to change the expanders to honour the predicate.
> 

That's true; before Richard Sandiford's recent reorg, we would assume
that any value that didn't match could be pulled into a register and
we wouldn't re-verify.

But that should be fixed now.


r~
Henderson, Stuart - April 4, 2011, 4:04 p.m.
Yep, I'm seeing this behaviour (getting the error using your patch).  But I'm confused as to why the define_expand is being considered when the predicate doesn't match. Apologies if this is a dim question, I'm still learning.

-----Original Message-----
From: Bernd Schmidt [mailto:bernds@codesourcery.com]
Sent: 31 March 2011 17:02
To: Richard Henderson
Cc: Henderson, Stuart; gcc-patches@gcc.gnu.org
Subject: Re: [Patch] Bfin: Ensure rotrsi and rotlsi don't accept non-const INTVALS

On 03/31/2011 05:42 PM, Richard Henderson wrote:
>>      (rotate:SI (match_operand:SI 1 "register_operand" "")
>> -               (match_operand:SI 2 "immediate_operand" "")))]
>> +               (match_operand:SI 2 "const_int_operand" "")))]
>>    ""
>>  {
>> -  if (INTVAL (operands[2]) != 16)
>> +  if (GET_CODE (operands[2]) != CONST_INT || INTVAL (operands[2]) != 16)
>>      FAIL;
>
> The point was, that you'd not need the CONST_INT check anymore,
> because it's handled by the predicate.

I have a dim memory of the problem being that something didn't check the
predicate. Sure enough, with the patch below applied to a 4.3 tree, I get

/local/src/egcs/gcc-4_3-branch/gcc/testsuite/gcc.c-torture/execute/20020226-1.c:43:
internal compiler error: in gen_rotrsi3, at config/bfin/bfin.md:1632
Please submit a full bug report,
with preprocessed source if appropriate.
See <http://gcc.gnu.org/bugs.html> for instructions.
compiler exited with status 1
output is:
(reg:SI 193)

Stuart, try to identify whether it still happens. If so, a better fix
would be to change the expanders to honour the predicate.


Bernd
Bernd Schmidt - April 7, 2011, 12:22 p.m.
On 04/04/2011 06:04 PM, Henderson, Stuart wrote:
> Yep, I'm seeing this behaviour (getting the error using your patch).
> But I'm confused as to why the define_expand is being considered when
> the predicate doesn't match.

It's just a bug in expand. Your original patch (plus using
const_int_operand as suggested by rth) is OK for all branches where the
problem occurs. I found yesterday that this is PR39768, so make sure to
mention that in the ChangeLog.


Bernd

Patch

Index: ../gcc-4_3-branch/gcc/config/bfin/bfin.md
===================================================================
--- ../gcc-4_3-branch/gcc/config/bfin/bfin.md	(revision 147761)
+++ ../gcc-4_3-branch/gcc/config/bfin/bfin.md	(working copy)
@@ -1611,6 +1611,11 @@  (define_expand "rotlsi3"
 		   (match_operand:SI 2 "immediate_operand" "")))]
   ""
 {
+  if (GET_CODE (operands[2]) != CONST_INT)
+    {
+      debug_rtx (operands[2]);
+      abort ();
+    }
   if (INTVAL (operands[2]) != 16)
     FAIL;
 })
@@ -1621,6 +1626,11 @@  (define_expand "rotrsi3"
 		     (match_operand:SI 2 "immediate_operand" "")))]
   ""
 {
+  if (GET_CODE (operands[2]) != CONST_INT)
+    {
+      debug_rtx (operands[2]);
+      abort ();
+    }
   if (INTVAL (operands[2]) != 16)
     FAIL;
   emit_insn (gen_rotl16 (operands[0], operands[1]));