Dg assembly through templating - #124
jrper wants to merge 4 commits into
Conversation
Sorry, something went wrong.
Clean up for intel. Fixes for GCC 4.6
…ugfixes. A few more c macros.
| @@ -0,0 +1,356 @@ | |||
| #include "fdebug.h" | |||
| #undef ASSERT | |||
There was a problem hiding this comment.
Why are we redefining ASSERT here?
Sorry, something went wrong.
There was a problem hiding this comment.
The standard assert function uses the Fortran continuation operator, //, which the c preprocessor then sees as a c-style comment and strips from the the processed template file. There may be a way to prevent this at the command line but I couldn’t find a version which worked.
On 12 Jul 2016, at 17:18, Stephan Kramer notifications@github.com wrote:
In assemble/Construct_Momentum_Element_DG.F90:
@@ -0,0 +1,356 @@
+#include "fdebug.h"
+#undef ASSERTWhy are we redefining ASSERT here?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.
Sorry, something went wrong.
|
I think I've resolved the last batch of review, and have built successfully on intel and Ubuntu. Is there anything further, and could this have a buildbot queue please? |
Sorry, something went wrong.
Sorry, something went wrong.
|
Ok, some more review: Momentum_DG.F90 (the diff is too large so I can't make inline comments):
|
Sorry, something went wrong.
| end if | ||
| #ifdef GENERIC | ||
| if(move_mesh) then | ||
| !// In the declarations above we've assumed these |
There was a problem hiding this comment.
What's with the extra // comment markers that have been added here?
Sorry, something went wrong.
|
Finally, I'd like some more discussion on the names for the macro-ed functions. They're currently a bit inconsistent, e.g. NLOC_X vs P_FLOC. And names like NLOC are a bit fluidity-old-style to me, and not immediately obvious to people that are only familiar with the femtools interface. Also we should be a bit more consistent with what is assumed, e.g. we have: #define NLOC(variable,ele) ele_loc(variable,ele) #define FLOC(variable,face) face_loc(U,face) which means that all cases we use FLOC we assume it is velocity, but for NLOC we use it on both U and source so that in the generic case we can have source%mesh and U%mesh be different, but not for the specialized cases. Note that we also have NLOC(q_mesh, ele) which I think breaks some cases since q_mesh is the viscosity mesh and we don't check that q_mesh==U%mesh in the momentum_element_selector. May I suggest a naming scheme like: ELE_LOC_U, FACE_LOC_P, etc.? Then we should probably be consistent and always keep the variable name, i.e. have #define FACE_LOC_P(variable, ele) face_loc(variable, ele) Alternatively, we get rid of the variable altogether and have: ! generic #define ELE_LOC_U(ele) ele_loc(U, ele) #define ELE_LOC_SOURCE(ele) ele_loc(source, ele) #define FACE_LOC_P(face) face_loc(P, ele) ! specialized P1DG-P2 in 2D #define ELE_LOC_U(ele) 3 #define ELE_LOC_SOURCE(ele) 3 #define FACE_LOC_P(face) 3 This seems to be the most robust choice as it makes all assumptions explicit. |
Sorry, something went wrong.
This pull request implements some of @anguscr's modifications to the dg assembly code. These use the preprocessor and code templating to provide versions of the assembly routines matching commonly used option choices which have prior knowledge of the size of small arrays, This allows for more efficient memory allocation and more aggressive compiler optimisations.