Mark, can you put yourself in the shoes of someone with less context,
either yourself next year or the rest of us trying to figure out how and
why the code get the way it is.
$ git log --oneline master..madams/sor-opt
3d38f94 all working
98ecc339 all working
a9293ac added all sizes in forward sweep
5e846cf working with forward sweep split
de0acf9 getting back to good innode.c
47bb0e1 removing debugs
c2d9332 small change to ex54
1092a6a fixed timers
f9b6998 1st optimization
We can hardly tell anything from this summary. Look at the code to find
out the most basic information is not efficient. Could you please write
commit messages that are useful to the reader? (Compare to typical
commit messages in PETSc over the past few months.)
* 1a38143 (madams/sor-opt) Merge branch 'madams/sor-opt' of bitbucket.org:petsc/petsc into madams/sor-opt
| * 891f4ed removed C++ comments
| * ab0bb76 removed makefile bug
| * 3d38f94 all working
| * 98ecc339 all working
| * a9293ac added all sizes in forward sweep
| * 5e846cf working with forward sweep split
| * de0acf9 getting back to good innode.c
| * aacab20 ...
| * 47bb0e1 removing debugs
| * 928e65f debug
| * c2d9332 small change to ex54
| * 1092a6a fixed timers
| * f9b6998 1st optimization
* | 534a7fe 1st optimization
* cdb4a13 Merge branch 'jed/snes-linesearch-warning'
rather than replacing the original, which could have been done using:
$ git push origin +madams/sor-opt
The other issue is that the new commit message "1st optimization"
doesn't provide any explanation of what was done or why.
I fixed the whitespace and formatting errors, then split the commit into
three parts, and attempted to write useful commit messages.
The first commit contains the scalar AIJ code which I think is fine and
can be merged. The second has the Inode optimizations, but I'm not wild
about the manual unrolling that you're using. What evidence do you have
that the unrolled code is faster? It is definitely harder to read and
probably makes the object code larger. If it's break-even or worse
performance (possible), then it shouldn't be merged. The third commit
adds the KSPSetUp to the example. That should be unnecessary for
correctness, so why was it added?
Finally, you may want to change your commit email to LBL:
The existing inode routines are unrolled an i just cloned them. The last two tests that I sent out used I inodes. A 2D and a 3D problem. I'm confused, you seem to think I added unrolling but you can see that I just modified and cloned the existing code.
And the addition of KSPSetup to the test was to get timing for the solve more easily. This is such a trivail change to a test code that I did not think worth going through all of this formalism. Take it out if you wish.
Sorry, I misread the diff and thought you had added the unrolling along
columns. I'm still skeptical of its benefit, but it's clearly not a new
I thought the separate call to KSPSetUp was probably for profiling
reasons, but I wonder if we should move it outside of the KSPSolve
event. I don't like that the profile attributes time differently if the
user calls KSPSetUp themselves or lets KSPSolve do it.