Split initialization #14564

mppf · 2019-12-03T17:38:02Z

This PR adds support for split initialization of Chapel variables.

Resolves #14271 which contains design discussion.

When a variable is declared without an initialization expression
(including possibly without a type), the compiler will search for a
following assignment and make that assignment initialize the variable.
The following assignment must be the first usage of the variable on that
control path. It can be contained in a nested block or a conditional but
not in a loop, begin, on statement, or function.

For example:

var a;
a = 2; // equivalent to `var a = 2`

const r: MyGenericRecord;
r = new MyGenericRecord(1); // equivalent to
                            // const r:MyGenericRecord = new MyGenericRecord(1)

The implementation approach in the compiler is to adjust the
normalization of variable initialization. Additionally, at parse time,
mark variables that are declared without a type and without an
initialization expression as requiring split initialization.

At normalization time, the compiler searches for applicable assignment
statements (called with findInitPoints). If these are found, it
normalizes the declaration into PRIM_INIT_VAR_SPLIT_DECL and the
assignment statements into PRIM_INIT_VAR_SPLIT_INIT. Function
resolution then handles these.

This PR also includes some changes to misc.cpp for reporting
instantiations. In particular, the code previously stored an FnSymbol*
for the last function for which an error was printed. That can go wrong
if this function is deleted in the meantime. So, it now uses integer ids
instead.

Additionally, this PR includes changes to the representation of on
statements for local compilation. These are now represented with
PRIM_BLOCK_ELIDED_ON and changed into regular blocks in
createTaskFunctions.

PR #14594 contains the spec changes for this feature.

Future Work:

split initialization for param, type, const ref, and ref
split initialization on param, type, and refs #14565
issue split initialization and nested functions #14621 discusses the interaction between split initialization
and nested functions

Reviewed by @vasslitvinov - thanks!

full local testing
primers pass with GASNet
primers pass with numa locale model

vasslitvinov

I suggest spot-checking gasnet and NUMA before merging.

My review is to be continued.

compiler/AST/primitive.cpp

compiler/passes/checkParsed.cpp

vasslitvinov · 2019-12-07T05:41:38Z

compiler/passes/normalize.cpp

+    return;
+  }
+
+  // TODO: call findInitPoints


Is it still a TODO?
If so, how can it possibly work without calling it?
Please double-check or update the comment.

It's a TODO for future work for getting split init working for type aliases ( #14565)

compiler/passes/normalize.cpp

vasslitvinov · 2019-12-10T04:13:45Z

compiler/resolution/functionResolution.cpp

+                               "another initialization has type %s "
+                               "but this initialization has type %s",
+                               toString(dst->type),
+                               toString(srcType));


Is it reasonable to provide the other initialization's line number? Ex.

USR_FATAL_CONT(call, "Split initialization uses multiple types"); USR_PRINT(call, "this initialization has type %s" ...); USR_PRINT(the-other-AST, "but this initialization has type %s" ...);

I think it would be reasonable to do so, but creating such an error message is actually not easy given the AST structure here. In particular, the-other-AST is not easily available. So I would like to leave it alone for this PR (of course we can try to improve it in the future).

compiler/util/misc.cpp

vasslitvinov · 2019-12-10T04:18:12Z

test/classes/delete-free/lifetimes/borrow-escapes.good

@@ -2,7 +2,9 @@ borrow-escapes.chpl:45: In function 'bad1':
 borrow-escapes.chpl:48: error: Scoped variable global would outlive the value it is set to
 borrow-escapes.chpl:46: note: consider scope of r
 borrow-escapes.chpl:52: In function 'bad2':
-borrow-escapes.chpl:57: error: Scoped variable outer would outlive the value it is set to
+borrow-escapes.chpl:57: error: Scoped variable would outlive the value it is set to


I would keep the name of the variable in the first error line. I prefer clarity over brevity in this case.

The split initialization changes caused this difference in error message but I didn't think it worth investigating at the current time. I have other work adjusting the lifetime checker. If you like we can make an issue to revisit it.

(this can happen when the lifetime checker sees an offending variable that is a temp. It leaves the names of the temps off.)

test/classes/delete-free/undecorated-generic/cannot-default-init-c.good

vasslitvinov · 2019-12-10T04:28:23Z

test/functions/sungeun/procEqualsExternType.chpl

@@ -8,6 +8,8 @@ proc =(ref a: myExternRecord, b: int) {

 var ex0: myExternRecord;
 var ex1: myExternRecord;
+ex0;
+ex1; // avoiding copy-init


Is this pattern due to a specific initialization-related feature being tested?
If not, please mention it in #14271 as "now users will need to do / be aware of ..."

Yes, this test is checking = and so I was disabling the split initialization (because otherwise that would not run =).

compiler/passes/checkParsed.cpp

vasslitvinov

My main concern is about having to write the initial values where previously we relied on default initialization. Ideally, discuss this within the group before merging. I'd be OK to discuss it afterwards, though.

test/parsing/vass/line-numbers-after-newline-in-string.good

vasslitvinov · 2019-12-10T04:35:48Z

test/runtime/configMatters/comm/atomic-error-msgs.chpl

@@ -8,7 +8,7 @@ areal.and(1);

 var astring: atomic string;

-var aint: atomic int;
+var aint: atomic int = 0;


In this case, this is unfortunate to have to write =0.

Please create a follow-up issue to discuss the various scenarios where the users now have to specify the initial value explicitly. For purposes other than testing, that is. :)

Like the other test, this test is checking = on atomics. I initialized the variable in order to ensure that = continue to be called because that is what the test is checking. I don't see this as a big problem for user code. (Certainly authors of a record / library might write such tests.)

vasslitvinov · 2019-12-10T04:38:42Z

test/studies/amr/lib/amr/AMRHierarchy_def.chpl

@@ -407,7 +407,7 @@ proc AMRHierarchy.buildRefinedLevel ( i_refining: int )
  //---- Cluster ----

  var min_width: dimension*int;
-  min_width = 2;
+  min_width = min_width + 2;


It would make code more understandable if you wrote var min_width: dimension*int = 2;
assuming that works.

Nope, that does not work, and neither does min_width = 2. The initialization might be reasonable but that seems to be a topic for a different issue.

proc main() { param dimension = 2; var x: dimension*int; x = 45; writeln(x); }

does not compile.

The parser includes several optimizations around on statements. Ideally these would be later in compilation. One of these optimizations removes on statements entirely for single-local (non-numa local model) compiles. This commit adds a BlockInfo PRIM_BLOCK_ELIDED_ON which marks blocks that were parsed as on statements but need no longer function as such. This allows later passes (in particular normalize) to reason about on statements in a consistent manner. PRIM_BLOCK_ELIDED_ON is removed in createTaskFunctions.

mppf · 2019-12-12T16:05:02Z

My main concern is about having to write the initial values where previously we relied on default initialization. Ideally, discuss this within the group before merging. I'd be OK to discuss it afterwards, though.

We discussed further offline. The test changes along these lines are for tests trying to check assignment.

@vasslitvinov

Spec changes for split init Spec changes to go along with PR #14564. Reviewed by @vasslitvinov - thanks! - [x] full local testing

@lydia-duncan

Un-future inOnLocal and remove compopts from related tests Follow-on to PR #14564. Reviewed by @lydia-duncan - thanks! - [x] test/classes/initializers passes in local configuration - [x] test/classes/initializers passes in gasnet configuration

@vasslitvinov

Enable split init for params PR #14564 added split initialization for `var` / `const` variable declarations. This PR continues the effort by enabling the feature for `param` variable declarations. * updated isPrimitiveScalar to include complex (because there are complex params and so I'd think of complex as a "primitive scalar" type) to resolve a function resolution issue. The call site to this function could alternatively be adjusted. * updated the normalizer to allow split init for `param` and in general to use `PRIM_INIT_VAR` instead of `PRIM_MOVE` for initializing a `param` * updated `fits_in_mantissa` in function resolution to consider non-default-sized integral values * updated `resolveInitVar` in function resolution to handle initializing params and to check that a param is only ever initialized to one value * fixed a bug in `resolveMoveForRhsCallExpr` with minimal modules and removed a no-longer-necessary `USR_FATAL` (that interfered with other efforts here) * updated `resolvePrimInit` to not initialize numeric variables for split-init scenarios (for these, init is now handled at `PRIM_INIT_VAR_SPLIT_INIT` rather than at `PRIM_INIT_VAR_SPLIT_DECL`) * updated `postFoldMoveUpdateForParam` to avoid emitting a confusing error when initializing a param from an un-coercible type (using some less-than-ideal AST matching, but I had trouble coming up with a better solution) * add tests of split init of params and also of some param cases that were not working correctly previously Reviewed by @vasslitvinov - thanks! - [x] full local testing

@vasslitvinov

Enable split init for type aliases PR #14564 added split initialization for var / const variable declarations. This PR continues the effort by enabling the feature for type alias declarations. * fixes a problem with the split init implementation in doFindInitPoints where a conditional block not mentioning the variable in any way would disable split init and improves upon related error messages. * adjusts normalizeTypeAlias to handle split init * add tests of the problem mentioned above and of split initialization of type aliases Reviewed by @vasslitvinov with input from @cassella - thanks! - [x] full local testing

@vasslitvinov

Enable split init for ref and const ref PR #14564 added split initialization for var / const variable declarations. This PR continues the effort by enabling the feature for `ref` and `const ref` declarations. * Adjusts normalizeVariableDefinition to handle split-init for reference variables * Adjusts resolveMoveForRhsCallExpr to give a user error for references initialized with different types (which can happen with split-init and conditionals) * Adjusts resolvePrimInit to fix a problem with split initialization of class types * Improves checking in the lifetime checker for cases that came up in testing split-init and refs (see split-init-ref-lifetime-error.chpl ) * Adjusts CondStmt::foldConstantCondition to flatten blocks resulting from folded if-exprs (which resolves errors created by the additional lifetime checking and is related to issue #10879). Reviewed by @vasslitvinov - thanks! - [x] full local testing

@bradcray

Language evolution updates for 1.21 Updates the language evolution document with various language changes. See also PRs #14903 #14903 #15041 #14564. Reviewed by @bradcray - thanks!

mppf mentioned this pull request Dec 3, 2019

split initialization on param, type, and refs #14565

Closed

mppf requested a review from vasslitvinov December 6, 2019 17:30

mppf mentioned this pull request Dec 6, 2019

Spec changes for split init #14594

Merged

1 task

vasslitvinov reviewed Dec 7, 2019

View reviewed changes

vasslitvinov reviewed Dec 10, 2019

View reviewed changes

vasslitvinov approved these changes Dec 10, 2019

View reviewed changes

mppf mentioned this pull request Dec 11, 2019

split initialization and nested functions #14621

Closed

mppf added 23 commits December 12, 2019 09:41

Add containsSymExprFor

ff76594

Add PRIM_INIT_VAR_SPLIT

8f31265

Disable error for variable not initialized

9a1b40c

Simple normalizer changes

213fc78

Add split initialization (mostly disabled)

7f9eaf4

Enable split initialization for non-compiler variables

043feb6

Clear err_fn at end of pass

4526e04

Use 2 new primitives for split initialization

84b0bf6

Add a test of split initialization

2e9e526

Arrays/domains use default-init/= even for split init

b3c39cf

Improve split init errors

3809265

Add more tests

b60d043

Fix loop detection

85f9021

Stop normalizing differently based on FLAG_COMPILER_GENERATED

8574286

Improve split-init in normalize

07e606f

Tighten up error about type blocks

aad7d16

Use ID instead of FnSymbol* in case it is deleted

e34d05f

Fix nil checking for var x:owned = nil;

8bbe0d8

Fix init= in Buffers package module

c592486

Fix tests for new nilability behavior

861f4bd

Disable split init for global variables

d4a905d

Make findInitPoints handle some NULL situations

80904bb

mppf added 8 commits December 12, 2019 09:43

Fix check for init function

5640a06

Handle records with default init

5b7be2f

Update some more tests

338dea9

make doFindInitPoints handle start==NULL

cf791a0

Update some .good files again

f2ada25

Fix another .good

2a2bb54

Disable split initialization for functions used in inner functions

c41160e

Resopond to review feedback

33d96bb

mppf force-pushed the split-init branch from 9cccd7c to 33d96bb Compare December 12, 2019 14:45

mppf added 2 commits December 12, 2019 09:49

Fix error message

4a2429e

Fix some .good files for error message wording changes

300390f

mppf merged commit 81a7482 into chapel-lang:master Dec 12, 2019

mppf deleted the split-init branch December 12, 2019 16:24

mppf added a commit that referenced this pull request Dec 12, 2019

Merge pull request #14594 from mppf/split-init-spec

faec1cd

Spec changes for split init Spec changes to go along with PR #14564. Reviewed by @vasslitvinov - thanks! - [x] full local testing

mppf mentioned this pull request Dec 12, 2019

Un-future inOnLocal and remove compopts from related tests #14630

Merged

2 tasks

mppf mentioned this pull request Dec 16, 2019

Enable split init for params #14657

Merged

1 task

mppf mentioned this pull request Dec 17, 2019

Enable split init for type aliases #14662

Merged

1 task

mppf mentioned this pull request Dec 18, 2019

Enable split init for ref and const ref #14673

Merged

1 task

mppf mentioned this pull request Jan 9, 2020

Request: Allow deferred initialization of a variable #14271

Closed

mppf mentioned this pull request Mar 26, 2020

Language evolution updates for 1.21 #15341

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split initialization #14564

Split initialization #14564

mppf commented Dec 3, 2019 •

edited

Loading

vasslitvinov left a comment

vasslitvinov Dec 7, 2019

mppf Dec 11, 2019

vasslitvinov Dec 10, 2019

mppf Dec 11, 2019

vasslitvinov Dec 10, 2019

mppf Dec 11, 2019

mppf Dec 11, 2019

vasslitvinov Dec 10, 2019

mppf Dec 11, 2019 •

edited

Loading

vasslitvinov left a comment

vasslitvinov Dec 10, 2019

mppf Dec 11, 2019

vasslitvinov Dec 10, 2019

mppf Dec 11, 2019 •

edited

Loading

mppf commented Dec 12, 2019

Split initialization #14564

Split initialization #14564

Conversation

mppf commented Dec 3, 2019 • edited Loading

vasslitvinov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mppf Dec 11, 2019 • edited Loading

Choose a reason for hiding this comment

vasslitvinov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mppf Dec 11, 2019 • edited Loading

Choose a reason for hiding this comment

mppf commented Dec 12, 2019

mppf commented Dec 3, 2019 •

edited

Loading

mppf Dec 11, 2019 •

edited

Loading

mppf Dec 11, 2019 •

edited

Loading