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.

Sign up for GitHub

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

Jump to bottom

I/O and Destroying temporaries at the end of the block vs end of statement #11492

Closed
mppf opened this issue Oct 29, 2018 · 2 comments
Closed

I/O and Destroying temporaries at the end of the block vs end of statement #11492

mppf opened this issue Oct 29, 2018 · 2 comments
Assignees
@mppf
Labels
area: Language area: Libraries / Modules type: Design

Comments

@mppf
Copy link
Member

mppf commented Oct 29, 2018 •
edited

I was writing a program testing I/O and was surprised by its behavior.

proc main() {
  var tmp = opentmp();

  var A = [1,2,3,4];
  var B:[A.domain] int;

  writeln(A);
  tmp.writer().write(A);
  tmp.reader().read(B);
  writeln(B);
}

I was surpsied that B did not equal A at the end of this program. What's going on?

  • Chapel I/O on files is bufferred
  • The buffer not flushed until the channel is closed
  • The buffer is not consistent between different channels for the same file
  • The channel is closed automatically when it goes out of scope
  • In this case, the channel is a temporary
  • Temporaries are currently destroyed at the end of the enclosing block (end of main, here)

This is the first compelling case I've seen that might argue for destroying temporaries at the end of a statement rather than the end of a block. However there are other ways to handle this should we decide to support this pattern.

See also #9532 which includes some discussion of where to destroy temporaries.
@mppf mppf mentioned this issue Oct 31, 2018
Closed
@mppf
Copy link
Member Author

mppf commented Oct 31, 2018

see also #11534

@mppf mppf mentioned this issue Feb 12, 2019
Closed
@mppf mppf mentioned this issue Aug 12, 2019
Closed
@mppf mppf mentioned this issue Jan 23, 2020
Merged
1 task
@mppf mppf self-assigned this Jan 23, 2020
mppf added a commit that referenced this issue Jan 23, 2020
@mppf
Merge pull request #14804 from mppf/improve-expiring-value-analysis
c107679 
Improve expiring value analysis

This PR makes two improvements to expiring value analysis.

* It updates the analysis to consider variables mentioned in a defer 
  statement to expire at the end-of-block. This resolves issue #14797.
* It changes the logic for inserting into the map detecting captures. 
  Now, a capture will not be detected before the variable it is
  describing is initialized. This resolves a problem I had not noticed
  with the I/O example from issue #11492. I also updated the test to
  output the array that was read, so that it will be more obvious if this
  pattern fails again.

Resolves #14797.

Reviewed by @benharsh - thanks!

- [x] full local testing
@mppf mppf mentioned this issue Mar 3, 2020
Merged
1 task
mppf added a commit that referenced this issue Mar 4, 2020
@mppf
Merge pull request #15041 from mppf/simpler-deinit
5d253c1 
Use simpler rule for deinit point

This PR updates the compiler to use simpler rules for when a variable is
dead. The expiring value analysis (proposed in #13704) did not end up
being as useful as initially expected for copy elision. As a result we
are shifting to a simpler design for when copy elision can occur -
#14874. 

That leaves the expiring value analysis impacting the deinitialization
point (#11534 #11492) and the ownership transfer from non-nilable
checking (#13600). However the ownership transfer checking is not
significantly improved with expiring value analysis (it is approximately
as effective when handled by the must-alias analysis in the nil checker).
That leaves the deinit points.

Since a copy elision can cause a variable to be effectively dead in the
current scope after it occurs, there is some relationship between deinit
points and copy elision. That is why #13704 tried to address these two
together.

For example:

``` chapel
proc acceptsInIntent( in arg ) { ... }

proc test() {
  var x: MyRecord;
  ...
  acceptsInIntent(x); // copy elision occurs here
  // x is effectively dead in this scope
  // accesses through refs/borrows hopefully are compilation errors
  // but could be use-after-frees.
}
```

However since the copy elision rule is relatively simple, this PR changes
the rule for when a variable is dead to simply include the copy elision
rule. Besides that, it is based upon the simpler rule in #13704 /
#11534 (comment) .

Here is the rule as of this PR:

> A variable is dead:
>  * after copy elision if it occurred (after the last mention is
>    used to copy-initialize a variable or in intent argument)
>  * for the results of nested call expressions not involved in initializing
>    a user variable or reference after the end of the statement
>  * otherwise, at the end of the scope in which it is declared

There are two important things to note about the above rule:
 1. "involved in initializing a user variable or reference" includes
    through split-init
 2. the above rule applies to module-scope variables (aka global
    variables) as well as local variables. For global variables

Also note that `--no-early-deinit` is available to make all
temps deinited end-of-block.

This PR takes the following steps to implement the change to
deinitialization points:
 * add a shared helper isInitOrReturn to CallExpr.cpp; use it in
   possiblyInitsDestroyedVariable
 * removes --report-expiring flag and the entire expiring value analysis
 * adjusts the normalizer to leave temps in module init functions
   (instead of making them global variables) so that this can be
   determined later (after split-inits are known)
 * fixes a few bugs with nil checking
 * adds MarkTempsVisitor / gatherTempsDeadLastMention /
   markTempsDeadLastMention which are called from fixPrimInitsAndAddCasts
   in resolveFunction.cpp. This analyzes temp variables by how they are
   used to determine if they are used to initialize a user variable.
   Besides adding `FLAG_DEAD_LAST_MENTION` or `FLAG_DEAD_END_OF_BLOCK`,
   if the variable is in a module initialization function, it will pull
   it out into global scope.
 * insertReturnTemps marks the new temps since this runs after
   MarkTempsVisitor
 * adjust several tests that have use-after-frees according to the new
   rules:
     * nbody_orig_1.chpl
     * assignRecord.chpl
     * owned-class-instantiation-types-user-init.chpl
     * owned-class-instantiation-types.chpl
 * (and these also which were added after checking in PR  #15072 detected an issue with them)
    * iter-return-first-borrowed-array-element.chpl
    * owned-record-instantiation-types-user-init.chpl 
    * classes/initializers/generics/inheritance/inherited.chpl 

Reviewed by @benharsh - thanks!

- [x] full local testing
@mppf
Copy link
Member Author

mppf commented Mar 10, 2020

PR #15041 implements a simpler rule for deinit points that enables this program to behave as originally expected.

@mppf mppf closed this as completed Mar 10, 2020
@mppf mppf mentioned this issue Aug 26, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@mppf mppf

Labels
area: Language area: Libraries / Modules type: Design
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@mppf