Re: target/5170: Supicious code in arm.md

Re: target/5170: Supicious code in arm.md

[pb]

Synopsis: Supicious code in arm.md

State-Changed-From-To: open->feedback
State-Changed-By: pb
State-Changed-When: Tue Jan 22 03:04:13 2002
State-Changed-Why:
    Seems there is no bug here.
    Submitter, are you satisfied now that the code is correct?

http://gcc.gnu.org/cgi-bin/gnatsweb.pl?cmd=view%20audit-trail&database=gcc&pr=5170

Re: target/5170: Supicious code in arm.md

[rearnsha]

Synopsis: Supicious code in arm.md

State-Changed-From-To: open->closed
State-Changed-By: rearnsha
State-Changed-When: Fri Mar 15 09:18:19 2002
State-Changed-Why:
    Comment added.

http://gcc.gnu.org/cgi-bin/gnatsweb.pl?cmd=view%20audit-trail&database=gcc&pr=5170

Re: target/5170: Supicious code in arm.md

[rearnsha]

Synopsis: Supicious code in arm.md

Responsible-Changed-From-To: unassigned->rearnsha
Responsible-Changed-By: rearnsha
Responsible-Changed-When: Thu Jan 24 02:30:27 2002
Responsible-Changed-Why:
    mine
State-Changed-From-To: feedback->open
State-Changed-By: rearnsha
State-Changed-When: Thu Jan 24 02:30:27 2002
State-Changed-Why:
    The code is correct, but since it has been queried it is 
    clear that a comment should be added at this point.

http://gcc.gnu.org/cgi-bin/gnatsweb.pl?cmd=view%20audit-trail&database=gcc&pr=5170

Re: target/5170: Supicious code in arm.md

[Klaus Pedersen]

The following reply was made to PR target/5170; it has been noted by GNATS.

From: Klaus Pedersen <klaus.k.pedersen@nokia.com>
To: pb@gcc.gnu.org, gcc-bugs@gcc.gnu.org, gcc-prs@gcc.gnu.org,
   nobody@gcc.gnu.org, gcc-gnats@gcc.gnu.org
Cc:  
Subject: Re: target/5170: Supicious code in arm.md
Date: Wed, 23 Jan 2002 20:07:24 +0100

 I probably do too much pattern-matching - to me it looked like a 
 classic coding error.
 
 But now that I know that it isn't, I am satisfied ;-)
 
 
 "ext pb@gcc.gnu.org" wrote:
 > 
 > Synopsis: Supicious code in arm.md
 > 
 > State-Changed-From-To: open->feedback
 > State-Changed-By: pb
 > State-Changed-When: Tue Jan 22 03:04:13 2002
 > State-Changed-Why:
 >     Seems there is no bug here.
 >     Submitter, are you satisfied now that the code is correct?
 > 
 > http://gcc.gnu.org/cgi-bin/gnatsweb.pl?cmd=view%20audit-trail&database=gcc&pr=5170

Re: target/5170: Supicious code in arm.md

[Richard Earnshaw]

The following reply was made to PR target/5170; it has been noted by GNATS.

From: Richard Earnshaw <rearnsha@arm.com>
To: Klaus.k.pedersen@nokia.com
Cc: gcc-gnats@gcc.gnu.org, Richard.Earnshaw@arm.com
Subject: Re: target/5170: Supicious code in arm.md 
Date: Fri, 04 Jan 2002 17:41:38 +0000

  
 > I think that there is a bug here:
 >  
 > >     for (i = 0; i < 25; i++)
 > >       if ((val & (mask << i)) == val)
 > >         break;
 > > 
 > >     if (i == 0)
 > >       FAIL;
 
 The test is correct.  We don't want to use the split pattern if we can 
 handle the constant with a single instruction.  That could lead to a 
 recursive split of the instruction.
 
 That is, the pattern is looking for cases of 
 
 	mov	Rn, #const
 
 that are invalid, but can be represented as
 
 	mov	Rn, #(const >> i)
 	lsr	Rn, #i
 
 Clearly we don't want to do this when i == 0, since a shift of zero is a 
 no-op (and recursive attempts to split the initial mov instruction would 
 never terminate).
 
 There is no need to test for i == 25, since the predicate for the pattern 
 has already told us that the constant is OK.
 
 
 

Re: target/5170: Supicious code in arm.md

[Richard Earnshaw]

The following reply was made to PR target/5170; it has been noted by GNATS.

From: Richard Earnshaw <rearnsha@arm.com>
To: Klaus.k.pedersen@nokia.com
Cc: gcc-gnats@gcc.gnu.org, Richard.Earnshaw@arm.com
Subject: Re: target/5170: Supicious code in arm.md 
Date: Fri, 04 Jan 2002 17:44:25 +0000

 > that are invalid, but can be represented as
 > 
 > 	mov	Rn, #(const >> i)
 > 	lsr	Rn, #i
 > 
 
 Err, I mean "lsl" not "lsr" of course.
 

target/5170: Supicious code in arm.md

[Klaus.k.pedersen]

>Number:         5170
>Category:       target
>Synopsis:       Supicious code in arm.md
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    unassigned
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Fri Dec 21 06:56:00 PST 2001
>Closed-Date:
>Last-Modified:
>Originator:     Klaus Pedersen
>Release:        gcc version 3.1 20011203 (experimental)
>Organization:
>Environment:
nice
>Description:
See also target/5107, investigated by Richard Earnshaw <rearnsha@arm.com>

The arm.md file defines the split pattern
 
 (define_split 
   [(set (match_operand:SI 0 "register_operand" "")
         (match_operand:SI 1 "const_int_operand" ""))]
   "TARGET_THUMB && CONST_OK_FOR_THUMB_LETTER (INTVAL (operands[1]), 'K')"
   [(set (match_dup 0) (match_dup 1))
    (set (match_dup 0) (ashift:SI (match_dup 0) (match_dup 2)))]
   "
   {
     unsigned HOST_WIDE_INT val = INTVAL (operands[1]);
     unsigned HOST_WIDE_INT mask = 0xff;
     int i;
     
     for (i = 0; i < 25; i++)
       if ((val & (mask << i)) == val)
         break;
 
     if (i == 0)
       FAIL;
 
     operands[1] = GEN_INT (val >> i);
     operands[2] = GEN_INT (i);
   }"


I think that there is a bug here:
 
>     for (i = 0; i < 25; i++)
>       if ((val & (mask << i)) == val)
>         break;
> 
>     if (i == 0)
>       FAIL;
 
Consider "val = 0x55", which can be represented as 0x55 << 0, but
calling the function with this value causes the function FAIL to
be called.
 
On the other hand, FAIL isn't called when passing the constant 0x101, 
which can't be represented, therefor I think that:
 
>     if (i == 0)
>       FAIL;
 
should read:
 
>     if (i == 25)
>       FAIL;
 
 
BR, Klaus Pedersen
 
>How-To-Repeat:
Well I have to admit it - I haven't been able to find
code affected by this typo. So if it is redundant - it
probably should be removed.
>Fix:
change 
   if (i == 0)
to 
   if (i == 25)
>Release-Note:
>Audit-Trail:
>Unformatted: