diff mbox series

amdgcn, libm: fix infinite loop

Message ID 20250807130941.363722-1-ams@baylibre.com
State New
Headers
Series amdgcn, libm: fix infinite loop |

Commit Message

Andrew Stubbs Aug. 7, 2025, 1:09 p.m. UTC 
  The end condition on this loop, unlike all the other similar loops, is
"i >= 0", which is a problem because "i <<= 1" can go negative and then zero if
you continue shifting, and so back to true, again.  This isn't a problem for
the loop in the scalar implementation, but it means we need to mask the shift
in the vector implementation.

This fixes GCC PR#121392.
---
 newlib/libm/machine/amdgcn/v64sf_fmod.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Comments

Thomas Schwinge Aug. 8, 2025, 8:14 a.m. UTC | #1 
Hi Andrew!

On 2025-08-07T13:09:41+0000, Andrew Stubbs <ams@baylibre.com> wrote:
> The end condition on this loop, unlike all the other similar loops, is
> "i >= 0", which is a problem because "i <<= 1" can go negative and then zero if
> you continue shifting, and so back to true, again.  This isn't a problem for
> the loop in the scalar implementation, but it means we need to mask the shift
> in the vector implementation.
>
> This fixes GCC PR#121392.

Thanks, I confirm this fixes the issue.

I suggest you 'git push', or I can do it for you if you don't have access
to newlib Git -- or, I'll be happy to sponsor you to get access, as
maintainer of the GCN parts in newlib.
(<https://sourceware.org/cgi-bin/pdw/ps_form.cgi> has the instructions
for the latter.)


Grüße
 Thomas


>  newlib/libm/machine/amdgcn/v64sf_fmod.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/newlib/libm/machine/amdgcn/v64sf_fmod.c b/newlib/libm/machine/amdgcn/v64sf_fmod.c
> index 7302420ad..b62b81929 100644
> --- a/newlib/libm/machine/amdgcn/v64sf_fmod.c
> +++ b/newlib/libm/machine/amdgcn/v64sf_fmod.c
> @@ -70,8 +70,11 @@ DEF_VS_MATH_FUNC (v64sf, fmodf, v64sf x, v64sf y)
>    v64si iy;
>    VECTOR_IF (hy < 0x00800000, cond)	// subnormal y
>      iy = VECTOR_INIT (-126);
> -    for (v64si i = (hy << 8); !ALL_ZEROES_P (cond & (i >= 0)); i <<= 1)
> -      VECTOR_COND_MOVE (iy, iy - 1, cond & (i >= 0));
> +    for (v64si i = (hy << 8); !ALL_ZEROES_P (cond & (i >= 0)); /* i <<= 1 */)
> +      {
> +	VECTOR_COND_MOVE (iy, iy - 1, cond & (i >= 0));
> +	VECTOR_COND_MOVE (i, i << 1, cond & (i >= 0));
> +      }
>    VECTOR_ELSE (cond)
>      VECTOR_COND_MOVE (iy, (hy >> 23) - 127, cond);
>    VECTOR_ENDIF
> -- 
> 2.50.0
Andrew Stubbs Aug. 8, 2025, 8:39 a.m. UTC | #2 
On 08/08/2025 09:14, Thomas Schwinge wrote:
> Hi Andrew!
> 
> On 2025-08-07T13:09:41+0000, Andrew Stubbs <ams@baylibre.com> wrote:
>> The end condition on this loop, unlike all the other similar loops, is
>> "i >= 0", which is a problem because "i <<= 1" can go negative and then zero if
>> you continue shifting, and so back to true, again.  This isn't a problem for
>> the loop in the scalar implementation, but it means we need to mask the shift
>> in the vector implementation.
>>
>> This fixes GCC PR#121392.
> 
> Thanks, I confirm this fixes the issue.
> 
> I suggest you 'git push', or I can do it for you if you don't have access
> to newlib Git -- or, I'll be happy to sponsor you to get access, as
> maintainer of the GCN parts in newlib.
> (<https://sourceware.org/cgi-bin/pdw/ps_form.cgi> has the instructions
> for the latter.)

I might be able to push (I can pull via ssh), but I'm not aware of 
having any authority to do so, in this project.

I normally post my patches and one of the maintainers approves and 
applies it.

I'm happy to take that task off their hands if they're happy for me to 
do so, but it's quite rare so I've never pushed for it.

Andrew
Jeff Johnston Aug. 8, 2025, 11:01 p.m. UTC | #3 
Patch applied.  Thanks.

-- Jeff J.

On Fri, Aug 8, 2025 at 4:15 AM Thomas Schwinge <tschwinge@baylibre.com>
wrote:

> Hi Andrew!
>
> On 2025-08-07T13:09:41+0000, Andrew Stubbs <ams@baylibre.com> wrote:
> > The end condition on this loop, unlike all the other similar loops, is
> > "i >= 0", which is a problem because "i <<= 1" can go negative and then
> zero if
> > you continue shifting, and so back to true, again.  This isn't a problem
> for
> > the loop in the scalar implementation, but it means we need to mask the
> shift
> > in the vector implementation.
> >
> > This fixes GCC PR#121392.
>
> Thanks, I confirm this fixes the issue.
>
> I suggest you 'git push', or I can do it for you if you don't have access
> to newlib Git -- or, I'll be happy to sponsor you to get access, as
> maintainer of the GCN parts in newlib.
> (<https://sourceware.org/cgi-bin/pdw/ps_form.cgi> has the instructions
> for the latter.)
>
>
> Grüße
>  Thomas
>
>
> >  newlib/libm/machine/amdgcn/v64sf_fmod.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/newlib/libm/machine/amdgcn/v64sf_fmod.c
> b/newlib/libm/machine/amdgcn/v64sf_fmod.c
> > index 7302420ad..b62b81929 100644
> > --- a/newlib/libm/machine/amdgcn/v64sf_fmod.c
> > +++ b/newlib/libm/machine/amdgcn/v64sf_fmod.c
> > @@ -70,8 +70,11 @@ DEF_VS_MATH_FUNC (v64sf, fmodf, v64sf x, v64sf y)
> >    v64si iy;
> >    VECTOR_IF (hy < 0x00800000, cond)  // subnormal y
> >      iy = VECTOR_INIT (-126);
> > -    for (v64si i = (hy << 8); !ALL_ZEROES_P (cond & (i >= 0)); i <<= 1)
> > -      VECTOR_COND_MOVE (iy, iy - 1, cond & (i >= 0));
> > +    for (v64si i = (hy << 8); !ALL_ZEROES_P (cond & (i >= 0)); /* i <<=
> 1 */)
> > +      {
> > +     VECTOR_COND_MOVE (iy, iy - 1, cond & (i >= 0));
> > +     VECTOR_COND_MOVE (i, i << 1, cond & (i >= 0));
> > +      }
> >    VECTOR_ELSE (cond)
> >      VECTOR_COND_MOVE (iy, (hy >> 23) - 127, cond);
> >    VECTOR_ENDIF
> > --
> > 2.50.0
>
>
diff mbox series

Patch

diff --git a/newlib/libm/machine/amdgcn/v64sf_fmod.c b/newlib/libm/machine/amdgcn/v64sf_fmod.c
index 7302420ad..b62b81929 100644
--- a/newlib/libm/machine/amdgcn/v64sf_fmod.c
+++ b/newlib/libm/machine/amdgcn/v64sf_fmod.c
@@ -70,8 +70,11 @@  DEF_VS_MATH_FUNC (v64sf, fmodf, v64sf x, v64sf y)
   v64si iy;
   VECTOR_IF (hy < 0x00800000, cond)	// subnormal y
     iy = VECTOR_INIT (-126);
-    for (v64si i = (hy << 8); !ALL_ZEROES_P (cond & (i >= 0)); i <<= 1)
-      VECTOR_COND_MOVE (iy, iy - 1, cond & (i >= 0));
+    for (v64si i = (hy << 8); !ALL_ZEROES_P (cond & (i >= 0)); /* i <<= 1 */)
+      {
+	VECTOR_COND_MOVE (iy, iy - 1, cond & (i >= 0));
+	VECTOR_COND_MOVE (i, i << 1, cond & (i >= 0));
+      }
   VECTOR_ELSE (cond)
     VECTOR_COND_MOVE (iy, (hy >> 23) - 127, cond);
   VECTOR_ENDIF