Skip to content

Fix amrex::Math for Microphysics#4570

Merged
ax3l merged 2 commits intoAMReX-Codes:developmentfrom
WeiqunZhang:sincos_is_simd
Jul 17, 2025
Merged

Fix amrex::Math for Microphysics#4570
ax3l merged 2 commits intoAMReX-Codes:developmentfrom
WeiqunZhang:sincos_is_simd

Conversation

@WeiqunZhang
Copy link
Copy Markdown
Member

@WeiqunZhang WeiqunZhang commented Jul 17, 2025

PR #4520 broke Microphysics's autodiff math. It could be fixed on the Microphysics side. However, we can fix it on the amrex side and we should use std::enable_if anyway to limit the data types.

@WeiqunZhang WeiqunZhang requested a review from ax3l July 17, 2025 01:25
@WeiqunZhang
Fix amrex::Math for Microphysics
275ab7e 
PR AMReX-Codes#4520 broke Microphysics's autodiff math. It could be fixed on the
Microphysics side. However, we can fix it on the amrex side and we should
use std::enable_if anyway to limit the data types.
@WeiqunZhang
Copy link
Copy Markdown
Member Author

WeiqunZhang commented Jul 17, 2025

@ax3l Could you test this?

@ax3l ax3l self-assigned this Jul 17, 2025
@ax3l ax3l added the bug label Jul 17, 2025
@ax3l ax3l mentioned this pull request Jul 17, 2025
Merged
16 tasks
@ax3l
Copy link
Copy Markdown
Member

ax3l commented Jul 17, 2025

This does not build with ImpactX:

/home/axel/src/amrex/Src/Base/AMReX_Math.H:162:26: error: template-id 'sincos<>' for 'std::pair<double, double> amrex::Math::sincos(double)' does not match any template declaration
  162 | std::pair<double,double> sincos (double x)
      |                          ^~~~~~
/home/axel/src/amrex/Src/Base/AMReX_Math.H:145:26: note: candidate is: 'template<class T_Real, typename std::enable_if<is_simd_v<T_Real>, int>::type <anonymous> > std::pair<_FIter, _FIter> amrex::Math::sincos(T_Real)'
  145 | std::pair<T_Real,T_Real> sincos (T_Real x)
      |                          ^~~~~~
/home/axel/src/amrex/Src/Base/AMReX_Math.H:177:24: error: template-id 'sincos<>' for 'std::pair<float, float> amrex::Math::sincos(float)' does not match any template declaration
  177 | std::pair<float,float> sincos (float x)
      |                        ^~~~~~
/home/axel/src/amrex/Src/Base/AMReX_Math.H:145:26: note: candidate is: 'template<class T_Real, typename std::enable_if<is_simd_v<T_Real>, int>::type <anonymous> > std::pair<_FIter, _FIter> amrex::Math::sincos(T_Real)'
  145 | std::pair<T_Real,T_Real> sincos (T_Real x)
      |                          ^~~~~~
/home/axel/src/amrex/Src/Base/AMReX_Math.H:216:26: error: template-id 'sincospi<>' for 'std::pair<double, double> amrex::Math::sincospi(double)' does not match any template declaration
  216 | std::pair<double,double> sincospi (double x)
      |                          ^~~~~~~~
/home/axel/src/amrex/Src/Base/AMReX_Math.H:198:26: note: candidate is: 'template<class T_Real, typename std::enable_if<is_simd_v<T_Real>, int>::type <anonymous> > std::pair<_FIter, _FIter> amrex::Math::sincospi(T_Real)'
  198 | std::pair<T_Real,T_Real> sincospi (T_Real x)
      |                          ^~~~~~~~
/home/axel/src/amrex/Src/Base/AMReX_Math.H:231:24: error: template-id 'sincospi<>' for 'std::pair<float, float> amrex::Math::sincospi(float)' does not match any template declaration
  231 | std::pair<float,float> sincospi (float x)
      |                        ^~~~~~~~
/home/axel/src/amrex/Src/Base/AMReX_Math.H:198:26: note: candidate is: 'template<class T_Real, typename std::enable_if<is_simd_v<T_Real>, int>::type <anonymous> > std::pair<_FIter, _FIter> amrex::Math::sincospi(T_Real)'
  198 | std::pair<T_Real,T_Real> sincospi (T_Real x)
      |                          ^~~~~~~~
gmake[2]: *** [_deps/localablastr-build/_deps/localamrex-build/Src/CMakeFiles/amrex_3d.dir/build.make:79: _deps/localablastr-build/_deps/localamrex-build/Src/CMakeFiles/amrex_3d.dir/Base/AMReX_BlockMutex.cpp.o] Error 1
gmake[1]: *** [CMakeFiles/Makefile2:2427: _deps/localablastr-build/_deps/localamrex-build/Src/CMakeFiles/amrex_3d.dir/all] Error 2
gmake: *** [Makefile:146: all] Error 2

@ax3l ax3l requested a review from zingale July 17, 2025 02:16
@WeiqunZhang
Copy link
Copy Markdown
Member Author

WeiqunZhang commented Jul 17, 2025

Maybe we need to use std::enable_if on the return type.

@ax3l
Copy link
Copy Markdown
Member

ax3l commented Jul 17, 2025

Yeah, I mean for SIMD the enable_if would be float or simd then allow, otherwise I think we have nothing to specialize the float/double variant to.

But I generally like to avoid enable_if because SFINAE are so costly in compile time.

@ax3l
Copy link
Copy Markdown
Member

ax3l commented Jul 17, 2025

The last commit I pushed compiles SIMD again, but I do not know how to test the microphysics issue.

@WeiqunZhang
Copy link
Copy Markdown
Member Author

WeiqunZhang commented Jul 17, 2025

Microphysics also works.

@ax3l ax3l merged commit 850a0fb into AMReX-Codes:development Jul 17, 2025
75 checks passed
@WeiqunZhang WeiqunZhang deleted the sincos_is_simd branch July 17, 2025 05:58
@WeiqunZhang
Copy link
Copy Markdown
Member Author

WeiqunZhang commented Jul 17, 2025

@ax3l The current overloads look strange. I think we should have something like below, which is more consistent with the standard.

#ifdef AMREX_USE_SIMD
template <typename T, std::enable_if_t<std::experimental::is_simd<T>,int> = 0>
std::pair<T,T> sincos (T const& x)  // use cons& like the standard
{ ... }
#end

// old way of not using templates
std::pair<double,double> sincos (double x) { return ... }

https://en.cppreference.com/w/cpp/numeric/math/sin.html

@WeiqunZhang
Copy link
Copy Markdown
Member Author

WeiqunZhang commented Jul 17, 2025

In the current implementation with AMREX_USE_SIMD, amrex::Math::sincos(3) is going to return a pair of integers. That's not good.

@zingale
Copy link
Copy Markdown
Member

zingale commented Jul 17, 2025

thanks!

@ax3l
Copy link
Copy Markdown
Member

ax3l commented Jul 18, 2025 •
edited
Loading

Agreed, yes we could just overload by argument type and template the simd overload.
#4570 (comment)

I must have had classes in mind when I wrote this function overload 😅

@WeiqunZhang
Copy link
Copy Markdown
Member Author

WeiqunZhang commented Jul 18, 2025

#4575

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ax3l ax3l ax3l approved these changes

@zingale zingale Awaiting requested review from zingale

Assignees

@ax3l ax3l

Labels

bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@WeiqunZhang @ax3l @zingale