Skip to content

Conversation

@mschollerer
Copy link
Contributor

@mschollerer mschollerer commented Apr 25, 2025

add fftw3 patch for neon as seen in this source repository local PR FFTW/fftw3#275 that will probaby never be merged there.

  • Changes comply with the maintainer guide.
  • SHA512s are updated for each updated download.
  • The "supports" clause reflects platforms that may be fixed by this new version.
  • Any fixed CI baseline entries are removed from that file.
  • Any patches that are no longer applied are deleted from the port's directory.
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Only one version is added to each modified port's versions file.

@mschollerer
Copy link
Contributor Author

mschollerer commented Apr 25, 2025

@microsoft-github-policy-service agree [company="{your company}"]

@microsoft-github-policy-service agree company="ifm electronic"

@Mengna-Li Mengna-Li added the category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist label Apr 25, 2025
@BillyONeal
Copy link
Member

#error:  "NEON only works in single precision on 32 bits ARM"

My understanding is that NEON is ARM32 only, not ARM64? I pushed a suggested edit, but given the title of this explicitly talking about ARM64 please look closely.

@BillyONeal
Copy link
Member

On the arm_neon_android failure presumably you would want that one to work so I think you need to look at that.

@BillyONeal BillyONeal marked this pull request as draft April 25, 2025 19:21
@mschollerer
Copy link
Contributor Author

mschollerer commented Apr 28, 2025

ah so its not so easy as fftw will be compiled as fftw3 (double) fftw3f (float) and fftw3l (long double) module. Neon is available for arm32 in single precision (float) and for arm64 (double).

We just used it on arm64 where everything worked fine, so arm32 will need some adoptions where I need to dig deeper, not sure when I can achieve that.

@mschollerer
Copy link
Contributor Author

mschollerer commented Apr 28, 2025

the easiest thing would be to adapt the module options on arm32 to turn NEON off for fftw3 (not fftw3f!). So there would need to be a change here for arm32 to disable NEON for double:

set(fftw3_options "")

what do you think @BillyONeal? its not very nice and I'm not sure if its possible, but it would make it work

@mschollerer mschollerer changed the title [fftw3] add neon support for aarch64 [fftw3] add neon support arm Apr 28, 2025
@mschollerer mschollerer changed the title [fftw3] add neon support arm [fftw3] add neon support Apr 28, 2025
@BillyONeal
Copy link
Member

what do you think @BillyONeal? its not very nice and I'm not sure if its possible, but it would make it work

Unfortunately I don't really know anything about 'fftw3' to make this kind of call :(. I can help express what is intended to be supported in a way vcpkg understands, but if the build is actually broken it seems that needs to be fixed first.

@BillyONeal BillyONeal force-pushed the mschollerer-add-fftw3-neon branch from 4e921c3 to 789b33a Compare April 28, 2025 19:43
@BillyONeal
Copy link
Member

I cut off my commit that blocked to arm32 and kept only the ci.feature.baseline.txt changes. Sorry for it being more complex than expected :)

@BillyONeal
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mschollerer
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 45222 in repo microsoft/vcpkg

@mschollerer
Copy link
Contributor Author

Looks like I cannot start the pipelines, @BillyONeal I did some reasonable changes that should fix hopefully all errors, can you please review my changes and trigger the pipelines again? Thanks a lot.

Copy link
Contributor

@dg0yt dg0yt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI doesn't run because you must resolve some conflicts.

@mschollerer
Copy link
Contributor Author

mschollerer commented May 8, 2025

CI doesn't run because you must resolve some conflicts.

ok, thanks for your support and suggestions. understood how things meant to be done. a small change was needed, but looks good from my side now

@mschollerer
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 45222 in repo microsoft/vcpkg

Copy link
Contributor

@dg0yt dg0yt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You shouldn't change ci.features.baseline.txt. (Different sections.)

And there still is a merge conflict, preventing CI.

@mschollerer
Copy link
Contributor Author

You shouldn't change ci.features.baseline.txt. (Different sections.)

And there still is a merge conflict, preventing CI.

ok, I'm not sure why @BillyONeal made a change to this?

@BillyONeal
Copy link
Member

BillyONeal commented May 8, 2025

ok, I'm not sure why @BillyONeal made a change to this?

Because I was trying to make the test pass: https://dev.azure.com/vcpkg/public/_build/results?buildId=115098

fftw3[core,openmp]:x64-windows-static resulted in the unexpected state pass after 1.8 min

@BillyONeal
Copy link
Member

I see, the feature-fails were grouped together and I undid that. Reverted that part.

@mschollerer mschollerer marked this pull request as ready for review May 9, 2025 11:13
@mschollerer
Copy link
Contributor Author

Looks good, thank you 🙂

symengine[tcmalloc](windows) = feature-fails # tcmalloc not found. See https://github.com/microsoft/vcpkg/issues/33576
tgui[sdl2] = options # At least one of the backend features must be selected: sdl2 sfml
vlfeat[openmp](osx) = feature-fails # No openmp on osx
vlfeat[openmp](osx) = feature-fails # No openmp on osxa
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking for a volunteer to remove the typo here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

volunteer found, done

@Mengna-Li Mengna-Li added the info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. label May 14, 2025
@JavierMatosD JavierMatosD merged commit 64810c6 into microsoft:master May 14, 2025
18 checks passed
@mschollerer mschollerer deleted the mschollerer-add-fftw3-neon branch May 27, 2025 12:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants