Dg assembly through templating

[jrper]

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.

[stephankramer]

Why are we redefining ASSERT here?

[jrper]

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 [email protected] wrote:

In assemble/Construct_Momentum_Element_DG.F90:

@@ -0,0 +1,356 @@
+#include "fdebug.h"
+#undef ASSERT

Why 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.

[jrper]

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?

[tmbgreaves]

Done and running:

http://buildbot.ese.ic.ac.uk:8080/builders/dg_assembly_through_templating

[stephankramer]

Ok, some more review:

Momentum_DG.F90 (the diff is too large so I can't make inline comments):

• line 48, each subroutine needs a comment describing what it does
• the indentation level of that subroutine is not consistent with the rest of the module
• line 59: spurious comment marker
• line 721: maybe add a comment we're now going into the construct_momentum_element_dg selected in momentum_element_selector()

[stephankramer]

What's with the extra // comment markers that have been added here?

[stephankramer]

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.