libCEED
libCEED copied to clipboard
fluids: Unify Newtonian Primtive functions
- Combines the separate primitive QFunctions in newtonian.h into a single function call.
- Replaces
StateFromQi*function pointers with QF helper functions using if statements inside. - Includes various other refactors and stylings.
Stripped this out from #1031 while that is a bit stalled for the moment. Figured it might as well be it's own PR
Thanks, this looks good from a coding standpoint. Can you do a basic test to identify whether there is any performance implications to this on CPU or GPU?
We're actually seeing a more significant performance difference that I was expecting. Running
./build/fluids-navierstokes -ceed /gpu/hip -options_file examples/fluids/channel.yaml -dm_plex_box_faces 20,20,1 -ts_max_steps 20 -output_freq 0
I'm seeing 3% slower for CPU (19.15 -> 19.75 seconds) and 0.5% for HIP on Noether (59.6 -> 59.9). This is trend is consistent after running a few more times.
I figured the branch prediction on the CPU would prevent it from being a significant factor there and any performance difference would be more evident on the GPU (though it very well could just be that at this problem size the consequential speed decrease is masked by other slow-downs).
Any idea why this is consequential by the case statements for the stabilization are (presumably) not? Or is there a way to tell the compiler that branch will be identical through a run (though I'd be surprised if there was a way to do that portably)?
A couple of code opinion/style questions:
-
I've kept the if statement versions of
StateFromQi. I think we had agreed that having these as slightly non-optimal for the boundary QFs is ok since they're not the dominant cost. I've still kept them here and only moved the volume QFs to the pointer method. We still good with that? -
For the function pointers passed into
[IFunction,IJacobian]_Newtonian_Generic, I've currently named aspStateFromQi(_fwd)emphasize that they're different than theStateFromQi*helper functions (pmeaning "pointer"). I originally had the function pointers named asStateFromQi*since they have the exact same interface and (intended) behavior as the helper functions. I'm open to switching back or giving them a different name. Thoughts?