Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Always use vectorization when the numerical scheme supports it #1752

Merged
merged 24 commits into from
Sep 26, 2022
Merged

Always use vectorization when the numerical scheme supports it #1752

Changes from 1 commit
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
6c53381
use the non physical counter in SIMD numerics
pcarruscag Sep 10, 2022
2edca53
automatically use vectorized Roe when possible
pcarruscag Sep 11, 2022
5f0e319
fix build and some doxygen cleanup
pcarruscag Sep 11, 2022
c841509
proper fix
pcarruscag Sep 11, 2022
3f6af89
this is what "using namespace std" does
pcarruscag Sep 11, 2022
004b67b
fix leak
pcarruscag Sep 11, 2022
b8515c7
MG segfault
pcarruscag Sep 11, 2022
ef4de7d
Merge remote-tracking branch 'upstream/develop' into vectorize_when_p…
pcarruscag Sep 11, 2022
41aa1d8
prevent nans with wall functions
pcarruscag Sep 12, 2022
ac58780
update hybrid regression
pcarruscag Sep 12, 2022
c808e8d
update parallel regression
pcarruscag Sep 12, 2022
1166497
serial and v&v tests
pcarruscag Sep 12, 2022
6a680ec
work around some forward mode issues
pcarruscag Sep 14, 2022
c0a8d7c
Merge remote-tracking branch 'upstream/develop' into vectorize_when_p…
pcarruscag Sep 17, 2022
4357145
regression updates and SIMD size 1 for reverse AD
pcarruscag Sep 18, 2022
37b4f58
updates after simd size of 1 for reverse AD
pcarruscag Sep 19, 2022
87f451e
Update SU2_CFD/include/numerics_simd/flow/convection/common.hpp
pcarruscag Sep 19, 2022
92fdc3d
Merge branch 'develop' into vectorize_when_possible
pcarruscag Sep 21, 2022
750def1
CoDiPack update.
jblueh Sep 23, 2022
59b5041
fix min/max problems? remove USE_VECTORIZATION from testcases
pcarruscag Sep 24, 2022
0b5991a
Merge remote-tracking branch 'upstream/vectorize_when_possible' into …
pcarruscag Sep 24, 2022
fedec9e
Revert "CoDiPack update."
pcarruscag Sep 24, 2022
f963ffd
convert the result of comparissons to passive type
pcarruscag Sep 25, 2022
c9af050
Revert "Revert "CoDiPack update.""
pcarruscag Sep 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions SU2_CFD/include/numerics_simd/flow/convection/common.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,10 +132,12 @@ FORCEINLINE CPair<ReconVarType> reconstructPrimitives(Int iEdge, Int iPoint, Int
break;
}
/*--- Detect a non-physical reconstruction based on negative pressure or density. ---*/
const Double neg_p_or_rho = fmax(fmin(V.i.pressure(), V.j.pressure()) < 0.0,
fmin(V.i.density(), V.j.density()) < 0.0);
/*--- Some weird issues with forward AD if this is all done in one line. ---*/
Copy link
Contributor

Choose a reason for hiding this comment

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

What happened exactly?

Copy link
Member Author

Choose a reason for hiding this comment

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

The result of the one line version was 1e-310, but I could not reproduce in a unit test, and also didnt get anything from valgrind. So i'm not sure, might have something to do with the expression types interfeering with each other?

Copy link
Member Author

Choose a reason for hiding this comment

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

Instead of 0 or 1.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I managed to reproduce it, I'll look into it a little further.

Copy link
Member Author

Choose a reason for hiding this comment

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

Awesome thanks

Copy link
Contributor

Choose a reason for hiding this comment

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

The std is not the point, just that writing

VecExpr::max_::operator[] (size_t i) -> /*auto deduced*/ { return codi::fmax(u[i], v[i]); }

requires the cmath version of fmax to be accessible like this, too. https://en.cppreference.com/w/cpp/numeric/math/fmax looks to me as if they are in std, though?

Copy link
Contributor

Choose a reason for hiding this comment

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

True, the suggested solution with passivedouble and fmin/fmax is specific to the su2double instantiation of the vector expressions. But maybe we could introduce some small traits that choose what I suggested for su2double and default to the previous solution for everything else?

Copy link
Member Author

Choose a reason for hiding this comment

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

Usually fmin and fmax work without namespace specification.
We can use fmin/max as the implementation for now and leave a note that they will not be efficient for integers.
I dont think we use that at the moment, Ill double check when I have some time and make the changes, thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

Argument-dependent lookup should select the CoDiPack overloads automatically. Alright.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the fix covers everything, active to passive, and suitable overloads.
Thank you for getting to the root cause.

const Double neg_p = fmin(V.i.pressure(), V.j.pressure()) < 0.0;
const Double neg_rho = fmin(V.i.density(), V.j.density()) < 0.0;
const Double neg_p_or_rho = fmax(neg_p, neg_rho);
/*--- Test the sign of the Roe-averaged speed of sound. ---*/
const Double R = sqrt(V.j.density() / V.i.density());
const Double R = sqrt(abs(V.j.density() / V.i.density()));
/*--- Delay dividing by R+1 until comparing enthalpy and velocity magnitude. ---*/
const Double enthalpy = R*V.j.enthalpy() + V.i.enthalpy();
Double v_squared = 0.0;
Expand Down