Add some constexpr values to the material declarations #1086
Conversation
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #1086 +/- ##
==========================================
- Coverage 13.80% 13.73% -0.07%
==========================================
Files 268 268
Lines 15007 15055 +48
==========================================
- Hits 2071 2068 -3
- Misses 12936 12987 +51 ☔ View full report in Codecov by Sentry. |
| @@ -30,6 +30,10 @@ format() { | |||
|
|
|||
| # NOTE: once the files of a directory are (almost) fully covered, consider moving it to allowlist_dir instead | |||
| local allowlist_file=" | |||
| src/Equations/elastic/Model/datastructures.hpp | |||
| src/Equations/viscoelastic2/Model/datastructures.hpp | |||
There was a problem hiding this comment.
What about viscoelastic(1)?
There was a problem hiding this comment.
Do we still support viscoelastic (1)? Otherwise, yes; I can change it.
There was a problem hiding this comment.
Wait—we don't have a Model/datastructures.hpp for viscoelastic (1). :)
There was a problem hiding this comment.
Yes, viscoelastic(1) is a prototype for a non-stiff pde with source term.
There was a problem hiding this comment.
We don't have a datastructure.hpp for viscoelastic(1), we just use the same as for viscoelastic2.
| def addStiffnessTensor(generator): | ||
| stiffnessTensor = Tensor('stiffnessTensor', (3, 3, 3, 3)) | ||
| direction = Tensor('direction', (3,)) | ||
| christoffel = Tensor('christoffel', (3,3)) | ||
|
|
||
| computeChristoffel = christoffel['ik'] <= stiffnessTensor['ijkl'] * direction['j'] * direction['l'] | ||
| generator.add('computeChristoffel', computeChristoffel) | ||
|
|
||
| def includeMatrices(matricesDir): | ||
| matrices = parseJSONMatrixFile('{}/sampling_directions.json'.format(matricesDir)) | ||
| return set([matrices.samplingDirections]) |
There was a problem hiding this comment.
This is anisotropy related, do we need it for all equations now?!
There was a problem hiding this comment.
It probably won't hurt too much to have it in... But I get it—I'll disable it if we don't have anisotropy enabled.
generated_code/generate.py
Outdated
| @@ -176,11 +180,50 @@ | |||
| except: | |||
| print('WARNING: ChainForge was not found. Falling back to GemmForge.') | |||
|
|
|||
| trueOutputDir = os.path.join(cmdLineArgs.outputDir, 'equation') | |||
There was a problem hiding this comment.
Why don't we use cmdLineArgs.equation here? This would pave the way for having more than one equation in the same binary?
There was a problem hiding this comment.
We could definitely do that. Originally, the equations were also templated, by SeisSol/yateto#62 (cf. what was done in #932; that's where most of this PR is from). It just doesn't completely work with the subroutine.cpp file yet.
There was a problem hiding this comment.
It's changed now to equation-{equation}-{order}-{precision}. The order part may be slightly overkill (as the matrices are usually pretty similar). But the equation partially and the precision surely would be necessary, I'd guess
| namespace='seissol', | ||
| gemm_cfg=gemmTools, | ||
| cost_estimator=cost_estimators, | ||
| include_tensors=include_tensors) | ||
|
|
||
| def generate_general(subfolders): | ||
| arch = useArchitectureIdentifiedBy('d' + cmdLineArgs.host_arch[1:]) |
There was a problem hiding this comment.
What about single precision here?
There was a problem hiding this comment.
That part was more meant to be precision-agnostic—i.e. the stiffness tensors etc.
(originally, it came from #932, to try to support multi-precision as well... Then forcing double was a good way to avoid templating in these classes as well)
There was a problem hiding this comment.
Makes sense. Can you add a comment that you use double here, since this kernel is not used at runtime?
There was a problem hiding this comment.
Done. In the same vein, couldn't we probably keep the anisotropy-related kernels (which are also only used during init); especially when going towards multi-material anyways?
src/Initializer/typedefs.hpp
Outdated
| struct MemoryProperties { | ||
| size_t alignment{ALIGNMENT}; | ||
| size_t pagesizeHeap{PAGESIZE_HEAP}; | ||
| size_t pagesizeStack{PAGESIZE_STACK}; | ||
| size_t alignment{Alignment}; | ||
| size_t pagesizeHeap{PagesizeHeap}; | ||
| size_t pagesizeStack{PagesizeStack}; | ||
| }; |
There was a problem hiding this comment.
Why do we need this struct? If all of these quantities are constexpr, we can always refer to them directly, insted of using this struct, which could be changed.
There was a problem hiding this comment.
They are used in the GlobalData allocation only—the values are set according to us being on host or device. We may be good with either keeping everything as is—or we could be moving the struct just there to src/Initializer/GlobalData.h.
There was a problem hiding this comment.
Then, I'd move the struct.
src/Model/plasticity.hpp
Outdated
| @@ -0,0 +1,40 @@ | |||
| #pragma once | |||
There was a problem hiding this comment.
How's our opinion on the non-standard #pragma once?
There was a problem hiding this comment.
I'll remove it. (I mainly use it for convenience first—and forgot to replace it here... ;) ... Maybe we'll need some, say, "create-me-an-empty-header-file" script sometime?)
A refactoring PR.
We just add values, like e.g. the number of quantities to the material declaration (or quantity names)—so that we can mid-term replace all the external preprocessor definitions, yielding a (hopefully) cleaner and more equation-independent code.
Also, we namespace some structs and make some tensors precision-independent.