Closed gmarkall closed 12 years ago
Looks like there is a bug in the generated code. There appear to be 6 quadrature points but i_g only loops over 3 of them.
The use of two quadrature loops rather than one also results in a much larger temporary array than is actually needed, but that's an optimisation we can fiddle with later.
I can't see any instance where "i_g < 3" occurs - I can find "i_g < 6" (the desired code) and "q_r_0 < 3" (also desired, it's a loop over basis functions inside the quadrature loop) - can you point me to a specific example?
The link in your comment above is the code I was looking at. It has loops with i_g<3
Thanks for spotting that! I had been fiddling around with the kernel and I'd accidentally introduced that bug when I was thinking about trying the kernel with three quadrature points. The MCFC code generation did produce loops with i_g < 6.
I've amended the code now, and run it again - the output from the MCFC-generated code produces very similar output to the original mass2d code now - it wasn't a difference in DOF ordering as I had thought, but was just because I had missed some of the quadrature points out.
Original mass2d output:
$ ./build/mass2d/mass2d_seq -mat_view
kernel routine with indirection: mass
row 0: (0, 0.250001) (1, 0.125001) (3, 0.125001)
row 1: (0, 0.125001) (1, 0.291668) (2, 0.0208334) (3, 0.145834)
row 2: (1, 0.0208334) (2, 0.0416668) (3, 0.0208334)
row 3: (0, 0.125001) (1, 0.145834) (2, 0.0208334) (3, 0.291668)
Mass2d with MCFC-generated kernel:
$ ./build/mass2d/mass2d_seq -mat_view
kernel routine with indirection: mass
row 0: (0, 0.25) (1, 0.125) (3, 0.125)
row 1: (0, 0.125) (1, 0.291667) (2, 0.0208333) (3, 0.145833)
row 2: (1, 0.0208333) (2, 0.0416667) (3, 0.0208333)
row 3: (0, 0.125) (1, 0.145833) (2, 0.0208333) (3, 0.291667)
The correct version of mass.h is at: https://github.com/gmarkall/OP2-Common/blob/f19a8f318df907b99cc4be83fdddd73690af8970/apps/c/mass2d/mass.h
Looks good to me, merging.
Summary of the changes:
i_r_*
, they are now calledq_r_*
- this is because the scope of the basis function indices in the OP2 matrix assembly kernels is now the entire kernel, rather than being constrained to loops inside the kernel. Although the scoping rules would always have allowed the correct index value to be used, it seemed confusing to have the same index name in two scopes that were nested. The quadrature loop basis indices are represented by the QuadratureBasisIndex class.;
when NullExpressions are part of the AST, a bugfix when unparsing pointers to arrays, and better feedback in the (now unused)getScopeFromNest()
function. Note that NullExpressions aren't actually inserted into the AST in this merge, although this was true at one point in some of the earlier commits to this branch.These tests have passed on the devtests-grm08 queues on the buildbot. Additionally, I tried substituting the mass kernel generated by MCFC into the mass2d example from Lawrence's OP2-Common repository. When com piled against the sequential library, and executed with
$ ./mass2d_seq -mat_view
, the matrix produced i s plausibly similar to the one produced by the original mass2d example, although there seems to be a diff erence in the numbering of DOFs between Fluidity and the mass2d example. See:https://github.com/gmarkall/OP2-Common/blob/f6422a0ff6953f4d4f7a570b245270122f59d975/apps/c/mass2d/mass.h
for the example that I tested with.