search

LeanderSchlegel / CRPropa3

The new CRPropa
GNU General Public License v3.0
0 stars 0 forks source link

SIMD Use #4

Closed JulienDoerner closed 2 years ago

JulienDoerner commented 2 years ago

Hey,

I have seen that in the PlaneWaveTurbulence the use of SIMD optimation can be disabled in the installation. This is not the case in our implementation of the tricubic interpolation. I think this could be a problem for those who don't have SIMD on there machines. I think to generelize the cmake flag FAST_WAVES could help here.

antoniusf commented 2 years ago

Hi!

Yes, I agree that it would be useful to make SIMD optional for the interpolation, too. When I cleaned up the plane waves implementation, I intentionally split the CMake flags into two: One for enabling SIMD in general, and one to enable fast waves on top of that. But I think you've already discovered this, going by your PR?

JulienDoerner commented 2 years ago

Hey. I have created this PR (#6), where I have introduced the HAVE_SIMD flag. This is set in the installation via the SIMD_EXTENSION. All other than "none" will enable SIMD for the TC Interpolation. I have thought about a own flag for the Interpolation (something like FAST_INTERPOLATION). But in contrast to the PlaneWaveTurbulence there is no way to use TC on Vectorgrids without the SIMD optimization. And I don't know any case where I want to use SIMD for tht PlaneWaveTurbulence but not for the TC Interpolation. Even in this case one could set the InterpolationType to something else. @antoniusf do you know a other case where it could be neccesarry? If I see it correct this are the same libraries used for PlaneWaveTurbulence and the interpolation, so there is no difference.

antoniusf commented 2 years ago

Hi – thanks for summarizing your PR, and sorry that I skipped the description of your PR (and looked only at the diff). I now understand what you meant.

Huh, no, if there's no way to use TC without SIMD, then I don't think the flag is sensible. I think its presence for fast waves comes from the fact that earlier in development, I considered the optimized version a lot more like an optional add-on (instead of being the only viable way to use plane waves). Also, I needed it to do performance comparisons.

Before Leander's response to #3, I'd have said that having a working TC for Grid3d's without precision loss issues might be desirable, but I don't think it's as important now.

If at some point someone does rework the genericization (such that a general non-simd TC becomes available) it might make sense to revisit this question (especially since then there'd be numeric differences for Grid3d), but even then I don't think there'll be much to gain with this extra flag.

JulienDoerner commented 2 years ago

Thanks for your comment. I think we agreed that we can continue with this PR right now and take a look at this issue later (in case a double based TC interpolation without SIMD will be made).