Re: 01/01: gnu: fftw: Build SIMD codelets.

Re: 01/01: gnu: fftw: Build SIMD codelets.

[Mark H Weaver]

Hi Eric,

ericbavier@centurylink.net (Eric Bavier) writes:

> bavier pushed a commit to branch core-updates
> in repository guix.
>
> commit 65bb22796f854cbc3eae053a80b1d64365dad376
> Author: Eric Bavier <bavier@cray.com>
> Date:   Fri Apr 6 10:53:06 2018 -0500
>
>     gnu: fftw: Build SIMD codelets.
>     
>     * gnu/packages/algebra.scm (fftw)[arguments]: Remove 'no-native phase; use
>     configure cache value instead.  Add configure flags for SIMD codelets.
>     (fftwf)[arguments]: Add neon configuration flag for 32-bit arm.
>     (fftw-avx): Remove variable.

[...]

> @@ -560,7 +569,10 @@ cosine/ sine transforms or DCT/DST).")
>      (arguments
>       (substitute-keyword-arguments (package-arguments fftw)
>         ((#:configure-flags cf)
> -        `(cons "--enable-float" ,cf))))
> +        (if (string-prefix? "arm" (or (%current-target-system)
> +                                      (%current-system)))
> +            `(cons "--enable-neon" ,cf)
> +            cf))))

Did you intend to remove the "--enable-float" configure flag in fftwf?
You didn't mention this change in the commit log.

The description of fftwf appends "Single-precision version" to the
description from fftw, but since your commit above, the fftw and fftwf
packages are identical except on armhf, as far as I can tell.  On armhf,
the build now fails with "configure: error: NEON requires single
precision".

  https://hydra.gnu.org/build/2674813/nixlog/1/tail-reload

Can you take a look?

      Mark

RE: 01/01: gnu: fftw: Build SIMD codelets.

[Eric Bavier]

Hi Mark,

> -----Original Message-----
> From: Mark H Weaver [mailto:mhw@netris.org]
> Sent: Thursday, May 03, 2018 6:26 PM
> To: Eric Bavier
> Cc: guix-devel@gnu.org
> Subject: Re: 01/01: gnu: fftw: Build SIMD codelets.
> 
> Hi Eric,
> 
> ericbavier@centurylink.net (Eric Bavier) writes:
> 
> > bavier pushed a commit to branch core-updates in repository guix.
> >
> > commit 65bb22796f854cbc3eae053a80b1d64365dad376
> > Author: Eric Bavier <bavier@cray.com>
> > Date:   Fri Apr 6 10:53:06 2018 -0500
> >
> >     gnu: fftw: Build SIMD codelets.
> >
> >     * gnu/packages/algebra.scm (fftw)[arguments]: Remove 'no-native
> phase; use
> >     configure cache value instead.  Add configure flags for SIMD codelets.
> >     (fftwf)[arguments]: Add neon configuration flag for 32-bit arm.
> >     (fftw-avx): Remove variable.
> 
> [...]
> 
> > @@ -560,7 +569,10 @@ cosine/ sine transforms or DCT/DST).")
> >      (arguments
> >       (substitute-keyword-arguments (package-arguments fftw)
> >         ((#:configure-flags cf)
> > -        `(cons "--enable-float" ,cf))))
> > +        (if (string-prefix? "arm" (or (%current-target-system)
> > +                                      (%current-system)))
> > +            `(cons "--enable-neon" ,cf)
> > +            cf))))
> 
> Did you intend to remove the "--enable-float" configure flag in fftwf?
> You didn't mention this change in the commit log.

Indeed, I think this was unintentional.

> Can you take a look?

Yes, thanks for checking.

`~Eric

Re: 01/01: gnu: fftw: Build SIMD codelets.

[Mark H Weaver]

Hi Eric,

Mark H Weaver <mhw@netris.org> writes:

> ericbavier@centurylink.net (Eric Bavier) writes:
>
>> bavier pushed a commit to branch core-updates
>> in repository guix.
>>
>> commit 65bb22796f854cbc3eae053a80b1d64365dad376
>> Author: Eric Bavier <bavier@cray.com>
>> Date:   Fri Apr 6 10:53:06 2018 -0500
>>
>>     gnu: fftw: Build SIMD codelets.
>>     
>>     * gnu/packages/algebra.scm (fftw)[arguments]: Remove 'no-native phase; use
>>     configure cache value instead.  Add configure flags for SIMD codelets.
>>     (fftwf)[arguments]: Add neon configuration flag for 32-bit arm.
>>     (fftw-avx): Remove variable.
>
> [...]
>
>> @@ -560,7 +569,10 @@ cosine/ sine transforms or DCT/DST).")
>>      (arguments
>>       (substitute-keyword-arguments (package-arguments fftw)
>>         ((#:configure-flags cf)
>> -        `(cons "--enable-float" ,cf))))
>> +        (if (string-prefix? "arm" (or (%current-target-system)
>> +                                      (%current-system)))
>> +            `(cons "--enable-neon" ,cf)
>> +            cf))))
>
> Did you intend to remove the "--enable-float" configure flag in fftwf?
> You didn't mention this change in the commit log.
>
> The description of fftwf appends "Single-precision version" to the
> description from fftw, but since your commit above, the fftw and fftwf
> packages are identical except on armhf, as far as I can tell.  On armhf,
> the build now fails with "configure: error: NEON requires single
> precision".
>
>   https://hydra.gnu.org/build/2674813/nixlog/1/tail-reload
>
> Can you take a look?

Another problem with the above commit has now become apparent.  In
addition to the 'fftwf' failure at configuration time on armhf, the
'fftw' package also now fails its test suite on our armhf build slaves.
While running the test suite, a test process is killed with SIGILL
(Illegal instruction).

I looked into it, and it appears that the "--enable-armv7a-cntvct"
configure flag that you added should be removed.  Adding it was a good
guess, but apparently the ARMv7-a CNTVCT instruction is not available
from user mode unless you use a specially patched kernel to enable it.

For details, see:

  https://github.com/FFTW/fftw3/blob/master/README-perfcnt.md
  https://github.com/thoughtpolice/enable_arm_pmu
  https://neocontra.blogspot.com/2013/05/user-mode-performance-counters-for.html

Since these issues are delaying the build-out of 'core-updates' on armhf
with ~1K dependency failures, and I'm reasonably confident what needs to
be done here, I'll go ahead and apply untested fixes for these issues to
'core-updates' soon.

     Regards,
       Mark

RE: 01/01: gnu: fftw: Build SIMD codelets.

[Eric Bavier]

Ok, if you feel confident, that's fine with me.

Otherwise, I have a patch that I've tested on x86-64, and in the middle of testing for armhf that I can push in a few hours.  I could easily add a commit that remove the "--enable-armv7a-cntvct" flag.

Eric Bavier, Scientific Libraries, Cray Inc.


> -----Original Message-----
> From: Mark H Weaver [mailto:mhw@netris.org]
> Sent: Tuesday, May 08, 2018 3:57 PM
> To: Eric Bavier
> Cc: guix-devel@gnu.org
> Subject: Re: 01/01: gnu: fftw: Build SIMD codelets.
> 
> Hi Eric,
> 
> Mark H Weaver <mhw@netris.org> writes:
> 
> > ericbavier@centurylink.net (Eric Bavier) writes:
> >
> >> bavier pushed a commit to branch core-updates in repository guix.
> >>
> >> commit 65bb22796f854cbc3eae053a80b1d64365dad376
> >> Author: Eric Bavier <bavier@cray.com>
> >> Date:   Fri Apr 6 10:53:06 2018 -0500
> >>
> >>     gnu: fftw: Build SIMD codelets.
> >>
> >>     * gnu/packages/algebra.scm (fftw)[arguments]: Remove 'no-native
> phase; use
> >>     configure cache value instead.  Add configure flags for SIMD codelets.
> >>     (fftwf)[arguments]: Add neon configuration flag for 32-bit arm.
> >>     (fftw-avx): Remove variable.
> >
> > [...]
> >
> >> @@ -560,7 +569,10 @@ cosine/ sine transforms or DCT/DST).")
> >>      (arguments
> >>       (substitute-keyword-arguments (package-arguments fftw)
> >>         ((#:configure-flags cf)
> >> -        `(cons "--enable-float" ,cf))))
> >> +        (if (string-prefix? "arm" (or (%current-target-system)
> >> +                                      (%current-system)))
> >> +            `(cons "--enable-neon" ,cf)
> >> +            cf))))
> >
> > Did you intend to remove the "--enable-float" configure flag in fftwf?
> > You didn't mention this change in the commit log.
> >
> > The description of fftwf appends "Single-precision version" to the
> > description from fftw, but since your commit above, the fftw and fftwf
> > packages are identical except on armhf, as far as I can tell.  On
> > armhf, the build now fails with "configure: error: NEON requires
> > single precision".
> >
> >   https://hydra.gnu.org/build/2674813/nixlog/1/tail-reload
> >
> > Can you take a look?
> 
> Another problem with the above commit has now become apparent.  In
> addition to the 'fftwf' failure at configuration time on armhf, the 'fftw'
> package also now fails its test suite on our armhf build slaves.
> While running the test suite, a test process is killed with SIGILL (Illegal
> instruction).
> 
> I looked into it, and it appears that the "--enable-armv7a-cntvct"
> configure flag that you added should be removed.  Adding it was a good
> guess, but apparently the ARMv7-a CNTVCT instruction is not available from
> user mode unless you use a specially patched kernel to enable it.
> 
> For details, see:
> 
>   https://github.com/FFTW/fftw3/blob/master/README-perfcnt.md
>   https://github.com/thoughtpolice/enable_arm_pmu
>   https://neocontra.blogspot.com/2013/05/user-mode-performance-
> counters-for.html
> 
> Since these issues are delaying the build-out of 'core-updates' on armhf
> with ~1K dependency failures, and I'm reasonably confident what needs to
> be done here, I'll go ahead and apply untested fixes for these issues to
> 'core-updates' soon.
> 
>      Regards,
>        Mark

Re: 01/01: gnu: fftw: Build SIMD codelets.

[Mark H Weaver]

Hi Eric,

Eric Bavier <bavier@cray.com> writes:

> Ok, if you feel confident, that's fine with me.
>
> Otherwise, I have a patch that I've tested on x86-64, and in the
> middle of testing for armhf that I can push in a few hours.  I could
> easily add a commit that remove the "--enable-armv7a-cntvct" flag.

Sorry for the duplicate work, but I already pushed my fixes in commit
69d5909e032e2fba57814ea9db52389d384d9341 to core-updates.  I tested them
on x86_64, but nowhere else.  It's not ideal, and I certainly wouldn't
do such a thing on 'master', but given the large number of affected
builds and my uncertainty about when you would fix it, I didn't want to
wait any longer.

FYI, I also removed "--enable-armv8-cntvct-el0" on 64-bit ARM, because
<https://github.com/FFTW/fftw3/blob/master/README-perfcnt.md> suggests
that as on 32-bit ARM, the relevant instruction is not normally
available from user mode.

I removed "--enable-mips-zbus-timer" on MIPS as well, because I suspect
that it's not supported on Loongson devices, although I'm not certain.

If you see any problems with my commit, please let me know.

     Thanks!
       Mark

Re: 01/01: gnu: fftw: Build SIMD codelets.

[Eric Bavier]

Hello Mark,

No problem.  I don't mind some parallel work that results in more confidence in the changes.

I saw your commit 69d5909, which looks good to me (you even untangled my nasty quoting, which I often can't seem to get right).

Sorry for the slow response, and thanks for finding the issue in the first place.

Eric Bavier, Scientific Libraries, Cray Inc.

________________________________________
From: Mark H Weaver <mhw@netris.org>
Sent: Tuesday, May 8, 2018 18:29
To: Eric Bavier
Cc: guix-devel@gnu.org
Subject: Re: 01/01: gnu: fftw: Build SIMD codelets.

Hi Eric,

Eric Bavier <bavier@cray.com> writes:

> Ok, if you feel confident, that's fine with me.
>
> Otherwise, I have a patch that I've tested on x86-64, and in the
> middle of testing for armhf that I can push in a few hours.  I could
> easily add a commit that remove the "--enable-armv7a-cntvct" flag.

Sorry for the duplicate work, but I already pushed my fixes in commit
69d5909e032e2fba57814ea9db52389d384d9341 to core-updates.  I tested them
on x86_64, but nowhere else.  It's not ideal, and I certainly wouldn't
do such a thing on 'master', but given the large number of affected
builds and my uncertainty about when you would fix it, I didn't want to
wait any longer.

FYI, I also removed "--enable-armv8-cntvct-el0" on 64-bit ARM, because
<https://github.com/FFTW/fftw3/blob/master/README-perfcnt.md> suggests
that as on 32-bit ARM, the relevant instruction is not normally
available from user mode.

I removed "--enable-mips-zbus-timer" on MIPS as well, because I suspect
that it's not supported on Loongson devices, although I'm not certain.

If you see any problems with my commit, please let me know.

     Thanks!
       Mark