-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix implicit solver compilation issues #1506
Conversation
bors try |
tryBuild failed: |
f907215
to
ae46d25
Compare
bors try |
tryBuild failed: |
bors try |
tryBuild failed: |
ae46d25
to
4f73269
Compare
bors try |
tryBuild succeeded! The publicly hosted instance of bors-ng is deprecated and will go away soon. If you want to self-host your own instance, instructions are here. If you want to switch to GitHub's built-in merge queue, visit their help page. |
bors try |
tryBuild succeeded! The publicly hosted instance of bors-ng is deprecated and will go away soon. If you want to self-host your own instance, instructions are here. If you want to switch to GitHub's built-in merge queue, visit their help page. |
59ce2ff
to
bd947df
Compare
bors try |
tryBuild succeeded! The publicly hosted instance of bors-ng is deprecated and will go away soon. If you want to self-host your own instance, instructions are here. If you want to switch to GitHub's built-in merge queue, visit their help page. |
bd947df
to
bf6c14a
Compare
bors try |
tryBuild failed: |
bors try |
tryBuild failed: |
bf6c14a
to
b9b04c3
Compare
901b05e
to
62fb219
Compare
62fb219
to
3fd7085
Compare
3fd7085
to
66abcb2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may want to revisit this at some point to see if we can eliminate the unroll functions, but overall looks fine to me.
Also, we can just ignore the invalidations issue (we can revisit this when we upgrade to julia 1.10) |
This PR fixes a number of compilation issues in the implicit solver infrastructure, which have been causing inference failures and allocations in the ClimaAtmos-based unit tests (and in ClimaAtmos.jl itself).
Content
MatrixFields
module with generatively unrolled functions, which gets rid of thestack overflow in type inference
compiler errors. Some of these functions are taken from Unrolled.jl, while others build upon the functions defined there. Move all unrolled functions intoUtilities.UnrolledFunctions
and add some documentation. Update theManifest.toml
files to add Unrolled.jl.FieldNameDict
so that there are no constantUnion
types being passed around, which gets rid of the allocations in the prognostic EDMF unit test. This involves mergingFieldMatrix
andFieldMatrixBroadcasted
into a single type, and also mergingFieldVectorView
andFieldVectorViewBroadcasted
. Add theis_lazy
function to distinguish between materialized and un-materializedFieldNameDict
s. Update all relevant documentation.norm_sqr(::FieldVector)
andarray_type(::FieldVector)
to make them type-stable for nestedFieldVector
s, which fixes the remaining type instabilities in a few unit tests. NestedFieldVector
s can be generated while debugging aStationaryIterativeSolve
that is part of a largerFieldMatrixSolverAlgorithm
.FieldNameSet
method definitions to avoid specializations of the formwhere {T}
, as was suggested by @charleskawczynski. This does not appear to affect compilation, but it does make the code a bit easier to read.Invalidations
Adding Unrolled.jl changes 4 rows in the invalidations table and brings the total number of invalidations up from 5529 to 5554. On the main branch, these 4 rows are
On this branch, these 4 rows are
Since the same function names appear in both tables, the increase in invalidations is probably not related to the other changes in this PR.