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

Should unused record variables be removed? #14758

Open
mppf opened this issue Jan 15, 2020 · 5 comments
Open

Should unused record variables be removed? #14758

mppf opened this issue Jan 15, 2020 · 5 comments

Comments

@mppf
Copy link
Member

mppf commented Jan 15, 2020

If a non-POD record is declared and never used, should the compiler emit a default-init/deinit pair, or just completely remove the record?

For example:

record R {
  proc init() { writeln("init"); }
  proc deinit() { writeln("deinit"); }
}

proc test() {
  var x: R;
}
test();

Here the variable x is unused, and the init and deinit functions have side effects.

Here we have three options:

  1. Decide that code declaring variables without using them is bad code. Warn the user that x is unused since that's probably unintentional.
  2. Dead-code eliminate x in the above example, so that it is neither initialized nor deinitialized. The program above would have no output.
  3. Run the initializer and de-initializer above for side effects. The program above would print out init and then deinit.

Although it might be bad style, what if a user wanted to create a "Computation" record that did all of the computation in its initializer? Would we be willing to eliminate that too? If not, why should a default-initializer be different?

As of PR #14691, the compiler uses option 3, but it is worth discussing if we want to use a different strategy.

@bradcray
Copy link
Member

I'd probably take the approach of leaving the variable in, calling its init and deinit, but warning the user that it's unused and having a flag to turn that warning off.

@vasslitvinov
Copy link
Member

The closest counterpart of (2) is moving a record from one variable to another:

var x: R;
var y = x;
// x is unused here

or

proc makeR() {
  var x = new R();
  return x;
}
var y = makeR();

where in each example the compiler could emit only one init/deinit pair instead of two.

@vasslitvinov
Copy link
Member

vasslitvinov commented Jan 16, 2020

I can see a couple of arguments in favor of (2).

  • From a software engineering perspective, the initializer and deinitializer are intended to have only the setup/tear-down computations. Where the "setup" sets things up for the bulk of the computation that will be executed later. If the user desires to run some computation with just a single statement, a type method - or a non-method function - is more appropriate than an unused variable.

  • No need to warn the user. If it matters that init+deinit are executed, it should be easy for the user to notice that they are not. Then they can change their code to execute the desired code in other ways.

Other than that, I do not have a preference among the listed options.

mppf added a commit that referenced this issue Jan 16, 2020
Adjust point of deinitialization based on expiring value analysis

This PR updates the compiler to use expiring value analysis in order to
choose the point at which `deinit` for a local record variable is run. It
does this according to the discussion in #13704.

This PR includes several minor modification to the rules described in
#13704.
1. Variables referred to in `begin` blocks are considered to expire at
   the end of the function (and issue #14750 discusses this and how it
   should interact with`sync` blocks)
2. An exception for `=` and `init=` for managed types. Because ownership
   transfer from non-nil is one of the use cases, we need the analysis to
   not consider `x` to potentially alias `y` in the below code, when both
   are `owned` - `var x = y;`. In general, if these are classes or
   records, they could share a reference to a field. For `owned` however,
   the right-hand-side of the initialization is left `nil` after the
   statement, and so there is no potential for aliasing between `x` and
   `y` after the statement. Additionally, for `shared`, while `x` and `y`
   refer to the same memory, this has no impact on when `x` or `y` can be
   deinitialized or a copy could be avoided. Therefore, when considering
   if a potential alias is created, the analysis ignores assignment and
   copy-initialization of managed types.

Additionally, it improves the split-init implementation. Previous to this
PR, an initialization point like `x = functionReturningByValue()` would
cause the compiler to emit `init=` when it did not need to. Additionally,
the compiler was emitting `deinit` calls for variables that were declared
but not yet initialized if there was a return from the block. Lastly,
this PR extends split-init to work with arrays (where before it was
disabled in resolution).

Lastly, it adjusts the lifetime checker to now require a lifetime clause
on `=` or `<=>` overloads for types that contain borrows.

Implementation steps:
* Remove FnSymbol::replaceReturnSymbol - it is unused
* rename `_statementLevelSymbol` to `chpl_statementLevelSymbol` and add
  `astr_chpl_statementLevelSymbol` to the compiler
* Print out variables stored directly in ForallStmts as "other variables"
  in the AST logs
* Add flags to disable split init and early deinit; change
  fLifetimeChecking to fNoLifetimeChecking to reflect the current default
* Move addAutoDestroyCalls to after ret-arg transform so it can run after
  lifetime checking (where the expiring value analysis runs) and make
  several adjustments to handle ret-args in addAutoDestroyCalls.
* Add code to normalize to mark the end-of-statement. Normalize adds a
  `PRIM_END_OF_STATEMENT` call after the end of each statement that isn't
  already at the end of a block or that refers to user variables.
  `PRIM_END_OF_STATEMENT` arguments are SymExprs referring to user
  variables that should be preserved until the end of that statement,
  which matters for code that is removed before callDestructors, such as
  `x.type`. Adjusted the unordered forall optimization and flatten
  functions to ignore these `PRIM_END_OF_STATEMENT` calls. These
  `PRIM_END_OF_STATEMENT` calls are removed in callDestructors.
* Updated the normalizer code for split-init to properly handle `x =
  makeRecord()` in addition to `x = new MyRecord()` and to store the
  type-expr in a temporary that is passed to both
  `PRIM_INIT_VAR_SPLIT_DECL` and `PRIM_INIT_VAR_SPLIT_INIT` to enable split
  init of arrays
* Updated the normalizer code for split init to explicitly handle
  TryStmts even though they currently disable split-init
* Updated functionResolution to handle split-init of arrays
* addAutoDestroyCalls creates a map from a statement to which variables
  should be destroyed there in order to implement the early deinit. Any
  variables not in the map will be destroyed according to previous rules
  (i.e. at the end of the block). Once deinit calls for these are added,
  the variable is added to the ignore set.
* updated addAutoDestroyCalls and AutoDestroyScope to track which
  variables have been initialized (in case they are split-inited) and to
  propagate that information to parent blocks (in case a variable is
  initialized with split-init in a nested block).

Test updates:
 * updating tests that check deinit occurs for end-of-block variables on
   return/throw/etc to force some variables to be end-of-block
 * work-arounds for #14746
 * updating tests for new deinit behavior and in some cases test both
   last-mention and end-of-block
 * new tests of new behavior

Reviewed by @vasslitvinov - thanks!

- [x] full local testing

Future Work:
 * document deinit points in the language specification -
   Cray/chapel-private#690
 * deinitialize in reverse initialization order rather than reverse
   declaration order - Cray/chapel-private#691

Related Design Questions:
 * Expiring value analysis interaction with `=` and `init=` for managed
   types (described above)
 * Lifetime checking and = overloads #14757 
 * Split-init interactions with deinitialization order #14747 
 * Should split-init be allowed within a try block? #14748 
 * Should last mention deinitialization point go inside blocks? #14749 
 * Expiring value analysis and begin blocks #14750 
 * Split-init interaction with warning about local-from-distributed
   #14746
 * Should unused record variables be removed? #14758
@cassella
Copy link
Contributor

cassella commented Jan 17, 2020

Would this apply if r's initialization was explicit (var r = new R();), but the variable were otherwise unused? That sounds like an RAII pattern. edit: Even with just the default-init, it could be RAII if the type was managing a single global lock.

Would the warning/optimization be done before or after param folding?

@mppf
Copy link
Member Author

mppf commented Jan 17, 2020

@cassella - as it is we're not planning to support things like RAII locks directly (at least not without adding other things) because we will deinitialize such variables before the end of the block (after PR #14691 and according to the proposal in #14691).

Nonetheless the "should it apply if initialization was explicit" question is what I was trying to get at with "If not, why should a default-initializer be different?". I think the answer might be different here if we are dead-code-eliminating vs. warning.

Would the warning/optimization be done before or after param folding?

If we did a warning, I'd expect it to happen before param folding. If we dead-code-eliminate, I'd expect it to happen after.

@mppf mppf self-assigned this Feb 6, 2020
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

4 participants