Skip to content
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

Adjust domain map implementations to no longer require pragma #16064

Merged
merged 19 commits into from
Jul 30, 2020

Conversation

mppf
Copy link
Member

@mppf mppf commented Jul 13, 2020

For issue #15673.
Also for issues #15703 and #8792.

This PR:

  • adjusts the domain map interface to include
    dsiElementDeinitializationComplete
  • removes pragma "no auto destroy" from domain map implementations
  • adds a user-facing capability to noinit arrays of POD elements

The goal of this PR is to resolve issue #15673 by adjusting the domain
map implementations to no longer require the use of a pragma "no auto destroy". Originally I was expecting to do that with a noinit
syntactic construction but it turns out that noinit is not strictly
necessary in this case since the buildArray(..., initElts=false)
pattern already covers it. Either way, the array in question has memory
allocated for elements but the elements are totally uninitialized.

In particular, to remove the need for pragma "no auto destroy", this PR
adds a deinitElts:bool field to DefaultRectangular to indicate if the
elements should be deinitialized when the array is destroyed. It adjusts
dsiElementInitializationComplete to set deinitElts to true. It adds
dsiElementDeinitializationComplete to set deinitElts to false (and
also allow similar behavior for other array types). It adjusts
dsiDestroyArr for to check this.deinitElts.

Even though noinit is not used in the distribution implementations,
this PR implements noinit for arrays of POD elements. For example,

var A:[1..10] int = noinit; // allocates space for elements but does not initialize them
for i in 1..10 {
  A[i] = i; // `=` is sufficient for initializing trivially copyable types
}

Users should also indicate when (if ever) the array's elements are
completely initialized so that the memory can be registered to support
communication. That is the subject of issue #16173.

In order to easily test no-init of arrays, this PR adds a flag
--allow-noinit-array-not-pod that will allow noinit of arrays of
non-POD elements. It uses this in several tests along with a version of a
low-level move module added to the test system. Issue #16172 discusses
the design of a user-facing low-level move module.

Reviewed by @vasslitvinov - thanks!

  • full local futures testing
  • primers pass with verify/valgrind and do not leak

@mppf mppf changed the title Implement noinit for arrays Adjust domain map implementations to no longer require pragma Jul 13, 2020
mppf added 18 commits July 30, 2020 11:15
When noinit will be used in the array implementation, it will not
be reasonable to turn it on and off with a flag.

Also remove test/compflags/lydia/noUseNoinit

The --no-use-noinit flag is removed so it does not make sense
to keep these tests.
So that these could be depending on a field for noinit cases.
See test/arrays/sparse/sparse.chpl
See test/types/records/ferguson/array/array-in-record2.chpl
See the new test and also
  test/distributions/bradc/assoc/userAssoc-array-domain-zipper.chpl
Adds the flag --allow-noinit-array-not-pod for use in noinit tests.
Copy link
Member

@vasslitvinov vasslitvinov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The domain map interface changes and the noinit capability should be as much a highlight of the PR message as not requiring the pragma. For one, even after removing this pragma, there are still two more pragmas to remove before we can claim the ability to rely on user-facing features only.

Code changes are fine.

@mppf mppf merged commit e25f2b3 into chapel-lang:master Jul 30, 2020
@mppf mppf deleted the array-noinit2 branch July 30, 2020 21:57
mppf added a commit that referenced this pull request Jul 30, 2020
Rebuild parser after noinit PR

PR #16064 made a small change to the parser but I forgot to commit the rebuilt parser.

This PR commits the rebuilt parser.

Trivial and not reviewed.
mppf added a commit that referenced this pull request Jul 31, 2020
…ization

Remove removeExcessDefaultInitialization.chpl

Follow-on to PR #16064. The test removeExcessDefaultInitialization was failing after that PR.

This test is a copy of

    test/expressions/noinit/assignAfterNoinit.chpl

so just remove it.

Test change only - not reviewed.
@mppf mppf mentioned this pull request Oct 1, 2020
mppf added a commit that referenced this pull request Oct 1, 2020
Add noinit technote

Follow-up to PR #16064.

The 1.23 release includes a very basic and restricted version of `noinit`
because we were able to agree on the behavior of that case.

The `noinit` feature is discussed in #15703 and #8792. The future work is
discussed in #16172, #16173.

Reviewed by @lydia-duncan - thanks!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants