Forall intents to operate on fields of 'this' #13479

vasslitvinov · 2019-07-16T06:33:08Z

This implements the suggestion in #13297 (comment)
specifically, "we don't think about the intent for this when considering
data within the loop, we just think about the intent for data
(as an array, its passed by reference)".

When a forall loop operates on the fields of the this formal, handle these
fields as if they are regular variables passed by the default intent.
This applies only when this is a record. See #13297 (comment)
for a discussion of what this entails.
We could consider this for classes as well - related to #10160.

The implementation uses ref temporaries. For example, it converts:

forall ... {
  this.data = i;
}

to

ref data_ref = this.data;
forall ... {  // 'data_ref' is passed by the default intent
  data_ref = i;
}

Cf. before this change the code above would be converted to:

forall ... with (const ref this) {
  this.data = i;  // 'this' is a shadow variable.
}

Some implementation details related to const?-ref intents are in d0d0825.

At present this is not implemented when this is of an array type,
even though the compiler considers it a "record".
For now, the behavior of task intents is unaffected.

Testing:

linux64 -futures
gasnet multilocale tests
numa

As suggested here: chapel-lang#13297 (comment) specifically, "we don't think about the intent for `this` when considering `data` within the loop, we just think about the intent for `data` (as an array, its passed by reference)". When a forall loop operates on the fields of the `this` formal, handle these fields as if they are regular variables passed by the default intent. This uses `ref` temporaries. This applies only when `this` is a record. For example, convert: forall ... { this.data = i; } to ref data_ref = this.data; forall ... { // 'data_ref' is passed by the default intent data_ref = i; } Cf. before this change the code above would be converted to: forall ... with (const ref this) { this.data = i; // 'this' is a shadow variable. } At present this is not implemented when `this` is of an array type, even though the compiler considers it a "record".

With this commit, in this code: proc MyRecord.myProc() { forall .... { ... use( this.myArrayField ) ...; } } the intent for `this` in `myProc` is 'ref' if use() is a write and 'const ref' if use() is a read. DETAILS With the previous commit 43cd18b, the compiler rewrites the above into: proc MyRecord.myProc() { ref myArrayField_ref = this.myArrayField forall .... with (ref myArrayField_svar(outerVar = myArrayField_ref)) { ... use( myArrayField_svar ) ...; } } One part of this change to cullOverReferences is to build the dependence from myArrayField_svar to myArrayField_ref upon seeing the relationship between these two as the shadow variable and the outer variable in the ForallStmt. The lack of this dependence was not an issue earlier because the compiler replaces references to a ref-intent shadow variable with references directly to its outer variable and discards the shadow variable, see 'prune' in resolveShadowVarTypeIntent(). For this change, I chose not to do so in order to start exercising ref-intent shadow variables in the rest of the compiler. Although I am not completely sure that **all** ref-intent shadow variables were pruned before. While "ref" in `myArrayField_ref` did not side-track cullOverReferences into forcing the ref-intent on the `this` formal, "ref" in myArrayField_svar did. Below I list the source and the AST at start of cullOverReferences for two cases: (A) an implicit shadow variable for an array field and (B) an array formal with const?-ref intent. symExprIsSet(), invoked in (A) on SymExpr 925541, used to return true just because `xx_svar` has FLAG_REF_VAR. Which it should, as it has a ref intent. When invoked in (B) on SymExpr 925548 in an otherwise-identical situation, returns false because `arg` is a formal and so does not have FLAG_REF_VAR even though it also has a ref intent. I apply FLAG_DEFAULT_INTENT_IS_REF_MAYBE_CONST to obtain the desired behavior in symExprIsSet() so that `this` formal is not considered modified in this case. ----------------------------------------------------------------------------- (A) an implicit shadow variable for an array field `xx` const RNG = 1..4; record RR { var xx: [RNG] int; } proc RR.method() { forall idx in RNG do writeln(xx[idx]); } var ff: RR; proc main { ff.method(); writeln(ff); } 181023 fn method[181023]:void[4] 181027 def ref arg intent-'const? ref' this[181025]:_ref(RR)[916895] 181024 { 924436 def ref xx_ref[924435]:_ref(_array())[923730] 924442 move( xx_ref[924435] call( fn xx[613954] arg intent-'const? ref' this[181025] ) ) 181040 forall 181042 def const idx[181041]:int(64)[13] in 924423 call( fn these[754299] RNG[181002] ) with 924449 def ref xx_svar[924446]:_array()[923704] 924445 xx_ref[924435] do 181039 { 535788 def ref call_tmp[535787]:_ref(int(64))[644823] 535790 move( call_tmp[535787] ...) -- see details: (535790 CallExpr move (535791 SymExpr 'ref call_tmp[535787]:_ref(int(64))[644823]') (925583 ContextCallExpr (925539 CallExpr (925581 SymExpr 'fn this[924951]:int(64)[13]') (925541 SymExpr 'ref xx_svar[924446]:_array()[923704]') (925542 SymExpr 'const idx[181041]:int(64)[13]')) (925560 CallExpr (925582 SymExpr 'fn this[925158]:_ref(int(64))[644823]') (925562 SymExpr 'ref xx_svar[924446]:_array()[923704]') (925563 SymExpr 'const idx[181041]:int(64)[13]')) (181036 CallExpr (925580 SymExpr 'fn this[924853]:_ref(int(64))[644823]') (924450 SymExpr 'ref xx_svar[924446]:_array()[923704]') (285214 SymExpr 'const idx[181041]:int(64)[13]')))) 929400 def val coerce_tmp[929398]:int(64)[13] 929402 move( coerce_tmp[929398] deref( call_tmp[535787] ) ) 181038 call( fn writeln[929380] coerce_tmp[929398] ) 181039 } 378387 return( _void[45] ) 181024 } ----------------------------------------------------------------------------- (B) an array formal `arg` with const?-ref intent const RNG = 1..4; type RR = [RNG] int; proc method(arg: RR) { forall idx in RNG do writeln(arg[idx]); } var ff: RR; proc main { method(ff); writeln(ff); } 181023 fn method[181023]:void[4] 181022 def ref arg intent-'const? ref' arg[181020]:_array()[924147] 181024 { 181035 forall 181037 def const idx[181036]:int(64)[13] in 924451 call( fn these[754626] RNG[181002] ) with() do 181034 { 536483 def ref call_tmp[536482]:_ref(int(64))[645150] 536485 move( call_tmp[536482] ...) (536485 CallExpr move (536486 SymExpr 'ref call_tmp[536482]:_ref(int(64))[645150]') (925590 ContextCallExpr (925546 CallExpr (925588 SymExpr 'fn this[924958]:int(64)[13]') (925548 SymExpr 'ref arg arg[181020]:_array()[924147]') (925549 SymExpr 'const idx[181036]:int(64)[13]')) (925567 CallExpr (925589 SymExpr 'fn this[925165]:_ref(int(64))[645150]') (925569 SymExpr 'ref arg arg[181020]:_array()[924147]') (925570 SymExpr 'const idx[181036]:int(64)[13]')) (181031 CallExpr (925587 SymExpr 'fn this[924860]:_ref(int(64))[645150]') (285198 SymExpr 'ref arg arg[181020]:_array()[924147]') (285199 SymExpr 'const idx[181036]:int(64)[13]')))) 929407 def val coerce_tmp[929405]:int(64)[13] 929409 move( coerce_tmp[929405] deref( call_tmp[536482] ) ) 181033 call( fn writeln[929387] coerce_tmp[929405] ) 181034 } 378372 return( _void[45] ) 181024 } -----------------------------------------------------------------------------

vasslitvinov · 2019-07-20T03:55:39Z

@mppf I followed up on our discussion and fixed cullOverReferences. Would you review?

mppf

Be sure to check the comm counts tests under GASNet

mppf · 2019-07-21T01:46:12Z

compiler/resolution/cullOverReferences.cpp

  }
  if (defOrUse & 2) { // use
-    ret |= symExprIsSetByUse(se);
+    if (symExprIsSetByUse(se))
+      return true;;


two semicolons

mppf · 2019-07-21T01:50:12Z

compiler/resolution/implementForallIntents.cpp

+      if (call->get(1)->getValType() == dtMethodToken)
+        // Field accesses are unresolved method calls at this point.
+        if (UnresolvedSymExpr* base = toUnresolvedSymExpr(call->baseExpr))
+          if (Symbol* fieldSym = recType->getField(base->unresolved, false))


Hmm... Looking for a field with the same name and assuming that the UnresolvedSymExpr would resolve to it seems a bit fraught to me. Nonetheless I think it is OK for now. It might be worth putting a comment that it might not be sufficient in the future (is that right? could one create parentheses-less methods that shadow the field, say?)

mppf · 2019-07-21T01:50:37Z

compiler/resolution/implementForallIntents.cpp

+static ShadowVarSymbol* createSVforFieldAccess(ForallStmt* fs, Symbol* ovar,
+                                               Symbol* field)
+{
+  bool isConst = ovar->isConstant() || field->isConstant();


mppf · 2019-07-21T01:51:54Z

compiler/resolution/cullOverReferences.cpp

@@ -243,7 +243,8 @@ static
 bool symExprIsSet(SymExpr* se)
 {
  // The ref is necessary if it is for an explicit ref var
-  if (se->symbol()->hasFlag(FLAG_REF_VAR)) {
+  if (se->symbol()->hasFlag(FLAG_REF_VAR)                             &&
+      ! se->symbol()->hasFlag(FLAG_DEFAULT_INTENT_IS_REF_MAYBE_CONST) ) {


This is fine, but I wouldn't have expected your shadow variables to have FLAG_REF_VAR set.

mppf · 2019-07-21T01:54:40Z

compiler/resolution/implementForallIntents.cpp

+  ShadowVarSymbol* svar = new ShadowVarSymbol(svarIntent,
+                                              astr(field->name, "_svar"),
+                                              new SymExpr(fieldRef));
+  svar->addFlag(FLAG_DEFAULT_INTENT_IS_REF_MAYBE_CONST);//to cullOverReferences


Is this copying flags from the other variable? AFAIK cull over references considers all references that don't have FLAG_REF_VAR to be ref-maybe-const. Anyway since it's not an argument and it's not on a type this seems like an abuse of FLAG_DEFAULT_INTENT_IS_REF_MAYBE_CONST.

Could you check if you can get it working by removing FLAG_REF_VAR, if it exists, in the right place? And if not, add a different flag FLAG_REF_MAYBE_CONST for this to use and for cullOverReferences to check?

Resolves chapel-lang#13577. This is the counterpart for task intents to chapel-lang#13479, which implemented the proposals in chapel-lang#13297 for forall-intents. Which is that ... When a task construct, i.e. `coforall`, `cobegin`, or `begin`, accesses a field of `this` that is a record, this field is treated as a variable passed into the task function by default intent. As opposed to passing `this` into the task function by default intent and accessing a field off of the corresponding task-private/shadow variable. The implementation reuses much of the code from chapel-lang#13479. Including the general approach of introducing a `ref` variable aliasing the record's field and passing that variable into the task function. I chose to place the new code in implementForallIntents.cpp because it is very similar to some code already there, doing similar work, and so that I can reuse a few helpers there without making them global. When `this` is referenced, implicitly or explicitly, in a task construct other than for a field access, it currently refers directly to the `this` formal, just as it does outside a task construct. Specifying a task intent explicitly for `this` or for an individual field is currently unsupported.

@mppf

Task intents to operate on fields of 'this' Resolves #13577. This is the counterpart for task intents to #13479, which implemented the proposals in #13297 for forall-intents. Which is that ... When a task construct, i.e. `coforall`, `cobegin`, or `begin`, accesses a field of `this` that is a record, this field is treated as a variable passed into the task function by default intent. As opposed to passing `this` into the task function by default intent and accessing a field off of the corresponding task-private/shadow variable. The implementation reuses much of the code from #13479. Including the general approach of introducing a `ref` variable aliasing the record's field and passing that variable into the task function. I chose to place the new code in implementForallIntents.cpp because it is very similar to some code already there, doing similar work, and so that I can reuse a few helpers there without making them global. When `this` is referenced, implicitly or explicitly, in a task construct other than for a field access, it currently refers directly to the `this` formal, just as it does outside a task construct. Specifying a task intent explicitly for `this` or for an individual field is currently unsupported. The purpose of the two existing tests modified here that write to field of `this` inside task-parallel constructs, I believe, is to ensure that `this` inside these constructs references the outer `this` correctly. My modifications preserve this intention. r: @mppf - thank you!

Update the spec now that task/forall intents operate on fields of `this` directly. See chapel-lang#13840 and chapel-lang#13479. While there, minor re-wording and commend cleanup. Add a tech note on overload sets checking. See chapel-lang#13442.

@mppf

Update task/forall intents in spec; add overload sets technote Updates the spec now that task/forall intents operate on fields of `this` directly. See #13840 and #13479. While there, minor re-wording and comment cleanup. Adds a tech note on overload sets checking. See #13442. r: @mppf

Restrict applicability of field intents #13297 proposed new semantics of task/forall intents w.r.t. fields of this when this is a record. It was implemented in #13479 and #13840. That implementation cast a wide net, applying the transformation for any type that isRecord() in the compiler, except arrays. However, #13297 talked only about fields of user-level records. This change makes the implementation apply transformation only to user-level records. Discussed with the group.

@dlongnecke-cray

Allow explicit task intents on `this` Helps with #19211, specifically #19211 (comment) . While this change does not enable setting default intents for record types, it implements the following: * allow users to specify a task intent on `this` explicitly * when there is an explicit intent on `this`, creation of implicit shadow variables for the fields of `this`, introduced in #13479, is disabled This PR implements this for both forall loops and task-parallel constructs, as these two have separate independent implementations (unfortunately, for historical reasons). Implementation notes ==================== Task intents for forall loops, formerly known as "forall intents": * Each shadow variable created for an explicit intent that is present in the source code has its `svExplicit` set to true. * When `this` is implicit in a field access in the source code, as in `f` meaning `this.f`, and there is an explicit shadow variable for `this`, scopeResolve makes `this` explicit, then doImplicitShadowVars() converts it to that shadow variable. * I changed resolveAndPruneExplicitShadowVars() to ensure that the shadow variable for `this` is not pruned if it had an explicit intent. Existing code would prune it if it had an explicit `ref` intent, and then the compiler would not be able distinguish between `this` with an explicit vs. implicit intent. * doConvertFieldsOfThis() is no longer invoked for the shadow variable corresponding to an explicit `this` intent. Task intents for coforalls and other task constructs: * pruneOuterVars() in createTaskFunctions.cpp no longer prunes `this` if it had an expicit intent. * As a result, createTaskFunctions.cpp converts references to `this` to references to a formal that is passed to the corresponding task function. This formal plays the role of a shadow variable within the task function. * As a result, convertFieldsOfRecordThis() no longer detects any references to `this` (see `ArgSymbol* thisArg = enclosingRecordThisArg(fn)`) and so no longer converts any field access to their own implicit shadow variables. r @dlongnecke-cray

vasslitvinov requested a review from mppf July 16, 2019 06:33

vasslitvinov mentioned this pull request Jul 19, 2019

forall inside record doesn't compile due to const ref this #13297

Closed

mppf approved these changes Jul 21, 2019

View reviewed changes

No need to set FLAG_REF_VAR on ShadowVarSymbols

9b6e07c

vasslitvinov merged commit 49e5dbd into chapel-lang:master Jul 25, 2019

vasslitvinov deleted the intents-for-fields-on-'this' branch July 25, 2019 02:00

vasslitvinov mentioned this pull request Jul 30, 2019

Task intents to operate on fields of 'this' #13577

Closed

vasslitvinov mentioned this pull request Aug 23, 2019

Task intents to operate on fields of 'this' #13840

Merged

vasslitvinov mentioned this pull request Sep 6, 2019

Update task/forall intents in spec; add overload sets technote #14019

Merged

vasslitvinov mentioned this pull request Sep 9, 2019

Restrict applicability of field intents #14057

Merged

3 tasks

vasslitvinov mentioned this pull request Sep 2, 2022

Allow explicit task intents on this #20601

Merged

3 tasks

vasslitvinov mentioned this pull request Sep 3, 2022

task intents for records and/or record fields #19211

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Forall intents to operate on fields of 'this' #13479

Forall intents to operate on fields of 'this' #13479

vasslitvinov commented Jul 16, 2019 •

edited

Loading

vasslitvinov commented Jul 20, 2019

mppf left a comment

mppf Jul 21, 2019

mppf Jul 21, 2019

mppf Jul 21, 2019

mppf Jul 21, 2019

mppf Jul 21, 2019

Forall intents to operate on fields of 'this' #13479

Forall intents to operate on fields of 'this' #13479

Conversation

vasslitvinov commented Jul 16, 2019 • edited Loading

vasslitvinov commented Jul 20, 2019

mppf left a comment

Choose a reason for hiding this comment

mppf Jul 21, 2019

Choose a reason for hiding this comment

mppf Jul 21, 2019

Choose a reason for hiding this comment

mppf Jul 21, 2019

Choose a reason for hiding this comment

mppf Jul 21, 2019

Choose a reason for hiding this comment

mppf Jul 21, 2019

Choose a reason for hiding this comment

vasslitvinov commented Jul 16, 2019 •

edited

Loading