-
Notifications
You must be signed in to change notification settings - Fork 443
Remove dmap and deprecated distributions
#28329
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
base: main
Are you sure you want to change the base?
Conversation
…sed dists --- Signed-off-by: Brad Chamberlain <[email protected]>
These utility records are designed to convert old layout classes into records by wrapping them, similar to what 'chpl_PrivatizedDistHelper' did for distribution classes. One is designed for rectangular arrays, the other for associative. --- Signed-off-by: Brad Chamberlain <[email protected]>
…rpose --- Signed-off-by: Brad Chamberlain <[email protected]>
--- Signed-off-by: Brad Chamberlain <[email protected]>
This mirrors what we've had for 'DefaultDist'. Neither of these are public symbols, but 'defaultDist' seems much more like the kind of thing we'd expose to users than 'DefaultDist' (in that we're not exposing class-based distributions anymore and there's no reason for the default distribution to be other than a singleton). --- Signed-off-by: Brad Chamberlain <[email protected]>
…ove-deprecated-dists
With this PR, BlockDist no longer contains a config const/var that was keeping it alive, so it is now considered dead and removed. This causes it not to be initialized since it's removed, and to slightly perturb the order of module init/deinit, requiring updates to printModuleInitOrder, countDeadModules, and deinit-order-modules. Interestingly, the comm-*.good files for the countDeadModules test also differed by one though my edit here changes by 4. I think this suggests that these tests are no longer being run in our ofi/ugni configurations, which seems likely to be given that we've dialed down system-specific full-suite testing as resources have been retired or changed. --- Signed-off-by: Brad Chamberlain <[email protected]>
--- Signed-off-by: Brad Chamberlain <[email protected]>
…ultDist --- Signed-off-by: Brad Chamberlain <[email protected]>
…direct uses --- Signed-off-by: Brad Chamberlain <[email protected]>
…instead In both cases, I used the pattern also used by most of our distributions now in which the record has a forwarding field to a record that handles the standard distribution interface forwarding to an old class-based implementation. In truth, there's no strong reason to preserve the class, but this approach requires fewer code changes and matches what we've done for other distributions. --- Signed-off-by: Brad Chamberlain <[email protected]>
--- Signed-off-by: Brad Chamberlain <[email protected]>
--- Signed-off-by: Brad Chamberlain <[email protected]>
jabraham17
left a comment
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.
Looks good!
Just noting that I still see mentions of dmap in the docs
- doc/rst/technotes/dsi.rst
- doc/rst/language/spec/domain-maps.rst
And those should probably be updated as well
| @@ -1 +1 @@ | |||
| dmapped-new-error-class.chpl:14: error: dmapped initialization expression requires a value, not a type - did you mean to use '<domain> dmapped new dmap(new <DistName>(<args>))'? | |||
| dmapped-new-error-class.chpl:20: error: dmapped initialization expression requires a value, not a type - did you mean to use '<domain> dmapped new dmap(new <DistName>(<args>))'? | |||
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.
This error message is now incorrect, there is no dmap a user can use
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.
Also a good catch! I'm going to remove this test (in addition to removing the compiler logic that printed new dmap(), as it was specific to a time when some, but not all, distributions were converted to classes.
While here, I also updated the error message to print the actual distribution type rather than <DistName> which seems like an obvious/simple improvement.
This PR removes the deprecated symbols related to distributions, layouts, and the
dmaptype. Specifically, it removes:dmaptypeBlockwas deprecated in favor ofblockDist)LayoutCSmoduleconfig constinBlockDist.chpl,disableAliasedBulkTransfer.In more detail:
I simply removed all deprecated distributions (
BlockCyclic,Block,Cyclic,DimensionalDist2D,Hashed,Private,Replicated,Stencil), associated comments, and the aforementionedconfigSimilarly, I removed the
LayoutCSmodule and all references to itThe
dmaptype was still used to create the logical singletondefaultDist, as it still provides the value-based wrapping around theDefaultDistclass. Ultimately,DefaultDistshould be rewritten to be a record, but here I took the simpler and more minimal approach of renaming thedmaptype tochpl_dmap, effectively making it an internal symbol. Beyond updatingdefaultDist's use of it, this also required updating Dyno to recognize the new name, as it is currently special-cased there.I removed a pair of helper routines in ChapelArray.chpl that existed merely to keep the
dmaptype working while deprecated.I removed tests in the
test/deprecateddirectory related to these featuresInterestingly, the removal of
disableAliasedBulkTransfercausedBlockDist.chplto go from being a non-dead module to a dead one; presumably, the presence of aconfigin a module prevents it from being dead-code eliminated since it represents a (potential) user-facing feature. This required updates to tests that were sensitive to module init/deinit order and counts of dead modules, such as:test/modules/sungeun/init/printModuleInitOrder.chpltest/modules/vass/deinit-order-modules.verbose.chpltest/optimizations/deadCodeElimination/elliot/countDeadModules.chplIn
SparseBlockDist.getDefaultSparseDist(), I changed a case that relied ondmapinto ahalt()because I believe it was only used for (old, now removed) class-based distributions, so should no longer be hit in practice. In essence, the halt() serves as an always-on assertion.I added a pair of records to wrap class-based distributions to
DSIUtil.chpl, similar to howchpl_PrivatizedDistHelperdid this for theBlock,Cyclic, etc. classes when they were being converted into records. One is for wrapping a rectangular distribution, the other for associative (chpl__rectLayoutHelperandchpl__assocLayoutHelper, respectively). At present, these are only used by tests in the test system that created custom distribution classes, though if we had class-based layouts, they could be used there as well to convert to record-based implementations. That said, if writing a new record-based distribution today, it should simply be written as a record, which would obviate the need for wrapping a class. Put another way, this is just a trick to minimize the amount of code that needs to be rewritten; with more effort, the tests that create custom distributions could/should be rewritten to simply use a record type as well.Tests with custom class-based distributions that required updates to use record-based distributions (and leverage the above helpers) included:
MyDist=>myDistintest/arrays/dmapped-new-error-class.chplMyDist=>myDistintest/distributions/bradc/wrongDomForDist/*.chplGlobalDistribution=>globalDistributionintest/domains/vass/no-dom-field-error.chplAccumStencil=>accumStencilintest/studies/comd/elegant/arrayOfStructs/util/*.chplI updated tests or code patterns that relied on
new dmap(new DefaultDist())to simply usedefaultDistinstead, such as intest/unstable/dmapped-sparse.chplandtest/users/engin/sparse_bulk/sparseBulkBoundsCheck.chplI also removed
test/distributions/diten/domainMethodNewDist.chplwhich seemed to serve no particular purpose in a dmap-less world