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

deinit point and param conditionals #14817

Closed
mppf opened this issue Jan 24, 2020 · 3 comments
Closed

deinit point and param conditionals #14817

mppf opened this issue Jan 24, 2020 · 3 comments

Comments

@mppf
Copy link
Member

mppf commented Jan 24, 2020

PR #14691 adjusts the deinitialization point according to expiring value analysis (which was discussed in #13704). This issue asks if folded-out param conditionals should impact the deinitialization point.

Consider for example the following program:

proc test() {
  var r = new R(1);

  writeln(r);

  // point 1

  writeln("before param conditional");

  if false {
    writeln(r);
  }

  // point 2

  writeln("after param conditional");
}
test();

Should the record deinitializer for r be run at point 1 or point 2?

In favor of point 1:

  • The compiler knows that r is not actually used after this point so can deinitialize it early, and generally freeing memory earlier is better

In favor of point 2:

  • A human programmer will more easily be able to predict the deinitialization point if it does not rely on compile-time constant folding and param conditional folding.

The current implementation does point 1, but we could extend param conditional folding to adjust PRIM_END_OF_STATEMENT calls to change it to point 2.

Note that split-init rules, which are conceptually related, apply to the code before param conditionals are folded.

@vasslitvinov
Copy link
Member

I support Point 2.

Given that our split-initialization detection is generally pre-resolution, we should stick with that in this case as well.

@mppf mppf self-assigned this Jan 24, 2020
@bradcray
Copy link
Member

I don't feel strongly about this, and am fine with sticking with the status quo.

@mppf
Copy link
Member Author

mppf commented Jan 28, 2020

There seems to be enough support for Point 2 so I am closing this issue. PR #14825 will make the change to the compiler. https://github.com/Cray/chapel-private/issues/690 tracks the TODO of updating the spec accordingly.

@mppf mppf closed this as completed Jan 28, 2020
mppf added a commit that referenced this issue Jan 29, 2020
Address deinit point and param loops and conditionals

Per the discussion in #14817, this PR updates the compiler so that the 
last-mention point for a variable mentioned in a folded-away param
conditional or loop is after that loop.

Reviewed by @benharsh - thanks! 

- [x] full local testing
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants