Hi Chapel Devs --
This is my long-awaited patch that removes formal temps. The heart of the
patch itself is pretty straightforward in functionResolution.cpp (and much
simpler than what was there before). The main work in getting the patch
to this point was dealing with other erroneous cases that it exposed.
There are two major remaining issues (noted in the message below), one
that I'm hoping Tom will look at (memory leak related, in iterators) and
one that I'm hoping Sung will (remote strings in tuples). Apart from
these two issues, the patch is ready to commit (and I'd be tempted to
commit it as-is).
-Brad
--------
Reduce the number of formal temps inserted by the compiler
At some point about six months ago, I found myself trying to figure
out why we insert so many formal temps (temps for formal arguments)
into the generated code; in looking into it, I was overwhelmed by the
logic in function resolution that determines whether or not we do. My
conjecture was that such temps should be required in only a very small
number of cases -- those requiring copies on the way into the
procedure that were not handled by the back-end function calls
themselves.
To that end, I tried ripping out all the logic and replacing it with
something far simpler: Insert a formal temp if the argument is 'inout'
or 'out' (because we pass by reference in these cases, so we need an
explicit temp); or for 'in or 'const in' intents if the type is one
for which we can't rely on C to do the copy for us.
This new logic worked surprisingly well, except that it unearthed some
pre-existing sins in the implementation that inserting formal temps
had previously been covering over. The main one of those was fixed by
my previous commit relating to remote value forwarding. This commit
addresses a few other smaller ones.
The benefits of this commit are: Simpler function resolution logic
w.r.t. formal temps; fewer formal temps inserted, reducing the size of
the AST (some of these were already eliminated by later passes like
copy propagation; others made it through to codegen and now don't);
fewer copies for routines that take record and tuple arguments where
formal temps had been inserted previously.
The most significant problem worth mentioning is that, by eliminating
formal temps, we eliminate a bunch of copy calls, which, for arrays,
contain calls to here.id. Programs that don't have any direct calls
to here.id (or other reasons to copy an array), started causing a pure
virtual function resolution error in the locale model hierarchy. The
reason this happens is that when no calls to here.id are resolved in
the user code, it only gets encountered while resolving autocopy
routines; yet for some reason, resolution at that point doesn't
properly determine that the routine requires dynamic dispatch so it
results in a static call to the "pure virutal" function instead.
I'm 95% certain that this is a pre-existing problem with function
resolution, yet it's one that I was unable to reproduce in a simpler
program. I worked around it here by putting a dumb reference to
here.id in a new module, ChapelDynDispHack.chpl, to make this specific
instance of the problem go away; presumably this will bite us for real
in a simpler program at some point and we'll have to deal with it
then. I don't think my changes have affected the behavior here in any
deep way, but rather that they simply reduced the number of copies
made in the program by eliminating temps, and therefore reduced the
likelihood of calls to here.id in the user code. The hack itself and
the simpler version of the test that demonstrates the issue
(test/classes/bradc/arrayInClass/arrayDomInClassRecord-simpler.chpl)
contain comments documenting this matter.
Two other odd cases that remain open issues:
* If we don't insert formal temps for the 'followThis' arguments that
are fed from leader to follower iterators, we start leaking memory
(as demonstrated by arrays/deitz/part7/test_descriptor_frees); I'm
not sure why this is and hope that Tom will investigate. I can
special-case these 'followThis' cases to close the leak (code to do
so currently commented out in functionResolution.cpp), but am not
wild about doing this without a good rationale.
* another ase has a tuple with a string element that's accessed across
(test/multilocale/diten/needMultiLocales/remoteStringTuple.chpl)
multiple locales and now fails; I'm proposing we not worry about
this case until after strings are converted into records, but want
Sung to look at this before I commit to see if she agrees.
Here are some additional file-by-file notes:
functionResolution.cpp: Contains the changes to the logic as described
above.
symbol.cpp: Fixes one of the formerly hidden sins, by flagging that
sync variables passed by blank intent are not constant.
ChapelBase.chpl: chpldev_refToString() used to be a hack that relied
on 'inout' intents; now that we have ref intents, it can (and, without
formal temps, needs to be) written more properly using a 'ref' intent.
refcnt.good: now that we don't put in formal temps for records, the
number of constructor/destructor calls in this program are now
significantly reduced (presumably a good thing), so the .good is
updated to reflect that.
test/*/*.chpl: removing formal temps revealed that some temps were
changing records (passed by 'const' intent by default) illegally. Our
const checking isn't good enough to flag these cases; and the formal
temps were preventing the modifications from showing up back at the
callsite; once they were removed, the problems became evident.
arrayDomInClassRecord-illegal.chpl: This future captures the case that
assignment to records passed by blank intent should flag an illegal
assignment error, but doesn't currently.
arrayDomInClassRecord-simpler.chpl: As noted above, this is the
simplified version of the code that causes the pure virtual function
error when there aren't user-level calls to here.id.
ChapelDynDispHack.chpl: And here's the module that contains the hack
to work around the issue.
Index: test/classes/bradc/arrayInClass/arrayDomInClassRecord-illegal.good
===================================================================
--- test/classes/bradc/arrayInClass/arrayDomInClassRecord-illegal.good (revision 22894)
+++ test/classes/bradc/arrayInClass/arrayDomInClassRecord-illegal.good (working copy)
@@ -1,2 +1 @@
-myR is: (D = {1..10}, A = 1.0 2.0 3.0 4.0 5.0 6.0 7.0 8.0 9.0 10.0)
-myC is: {D = {1..10}, A = 20.0 2.0 3.0 4.0 5.0 6.0 7.0 8.0 9.0 10.0}
+arrayDomInClassRecord-illegal.chpl:33: error: illegal lvalue in assignment
Index: test/classes/bradc/arrayInClass/arrayDomInClassRecord-simpler.chpl
===================================================================
--- test/classes/bradc/arrayInClass/arrayDomInClassRecord-simpler.chpl (revision 0)
+++ test/classes/bradc/arrayInClass/arrayDomInClassRecord-simpler.chpl (revision 0)
@@ -0,0 +1,36 @@
+record R {
+ var D: domain(1);
+ var A: [D] real = [i in D] i;
+}
+
+
+var myR = new R({1..10});
+
+writeln("myR is: ", myR);
+
+
+//
+// This is a test that doesn't work at present if the:
+//
+// assert(here.id >= 0)
+//
+// at the end of ChapelStandard.chpl is removed. The best explanation
+// for what's going wrong that I can find is that the only calls to
+// chpl_id()/here.id that are resolved in functionResolution.cpp occur
+// after the user code has been called and in resolveAutoCopies().
+// For some reason when the resolution occurs this late in the game,
+// it doesn't get the dynamic dispatch correct and dispatches to the
+// pure virual function in the root base class. I tried to reproduce
+// this in a standalone test with user-defined records and classes,
+// but had difficulty creating a similar type hierarchy that would
+// resolve a virtual function in resolveAutoCopies() but not
+// before. I don't know whether this is because I didn't get the
+// pattern right or whether the compiler inserts special code for
+// arrays/domains late in the compilation and therefore I can't
+// reproduce this myself. My hope is that it's the latter and that
+// once arrays/domains are less special, this will fix itself and
+// be removable. In either case, I'm 95% positive that this
+// is a pre-existing bug/condition that I've only now uncovered by
+// virtue of inserting fewer temps and therefore creating fewer array
+// copies/assignments, which cause there to be no other resolved
+// references to here.id in user code.
Index: test/classes/bradc/arrayInClass/arrayDomInClassRecord.chpl
===================================================================
--- test/classes/bradc/arrayInClass/arrayDomInClassRecord.chpl (revision 22894)
+++ test/classes/bradc/arrayInClass/arrayDomInClassRecord.chpl (working copy)
@@ -29,6 +29,6 @@
x(1) = y;
}
-proc baz(x,y) {
+proc baz(in x,y) {
x.A(1) = y;
}
Index: test/classes/bradc/arrayInClass/arrayDomInClassRecord-simpler.good
===================================================================
--- test/classes/bradc/arrayInClass/arrayDomInClassRecord-simpler.good (revision 0)
+++ test/classes/bradc/arrayInClass/arrayDomInClassRecord-simpler.good (revision 0)
@@ -0,0 +1 @@
+myR is: (D = {1..10}, A = 1.0 2.0 3.0 4.0 5.0 6.0 7.0 8.0 9.0 10.0)
Index: test/classes/bradc/arrayInClass/arrayDomInClassRecord-illegal.future
===================================================================
--- test/classes/bradc/arrayInClass/arrayDomInClassRecord-illegal.future (revision 0)
+++ test/classes/bradc/arrayInClass/arrayDomInClassRecord-illegal.future (revision 0)
@@ -0,0 +1,5 @@
+bug: const checking for record fields
+
+This test shows that we permit assignment to a record field passed
+by blank intent; such records should be treated as const, so their
+fields should not be assignable.
\ No newline at end of file
Index: test/users/ferguson/refcnt.good
===================================================================
--- test/users/ferguson/refcnt.good (revision 22894)
+++ test/users/ferguson/refcnt.good (working copy)
@@ -3,14 +3,10 @@
Done making global
Before scope
Initializing a RefCount
-In R initCopy {count = 1}
-Starting test_one {count = 2}
-In R.routine {count = 2}
-In R initCopy {count = 2}
-In test_two {count = 3}
-In ~R() 3
-Done test_one {count = 2}
-In ~R() 2
+Starting test_one {count = 1}
+In R.routine {count = 1}
+In test_two {count = 1}
+Done test_one {count = 1}
In ~R() 1
Destroying a RefCount
After scope
@@ -20,14 +16,10 @@
Starting R assign {count = 1}{count = 1}
Destroying a RefCount
Done R assign {count = 2}
-In R initCopy {count = 2}
-Starting test_one {count = 3}
-In R.routine {count = 3}
-In R initCopy {count = 3}
-In test_two {count = 4}
-In ~R() 4
-Done test_one {count = 3}
-In ~R() 3
+Starting test_one {count = 2}
+In R.routine {count = 2}
+In test_two {count = 2}
+Done test_one {count = 2}
In ~R() 2
In ~R() 1
Destroying a RefCount
@@ -35,44 +27,32 @@
Before scope (copy initialization 2)
Initializing a RefCount
In R initCopy {count = 1}
-In R initCopy {count = 2}
-Starting test_one {count = 3}
-In R.routine {count = 3}
-In R initCopy {count = 3}
-In test_two {count = 4}
-In ~R() 4
-Done test_one {count = 3}
-In ~R() 3
+Starting test_one {count = 2}
+In R.routine {count = 2}
+In test_two {count = 2}
+Done test_one {count = 2}
In ~R() 2
In ~R() 1
Destroying a RefCount
After scope (copy initialization 2)
Before scope (setting global)
Initializing a RefCount
-In R initCopy {count = 1}
-Starting test_one {count = 2}
-In R.routine {count = 2}
-In R initCopy {count = 2}
-In test_two {count = 3}
-Starting R assign {count = 1}{count = 3}
+Starting test_one {count = 1}
+In R.routine {count = 1}
+In test_two {count = 1}
+Starting R assign {count = 1}{count = 1}
Destroying a RefCount
-Done R assign {count = 4}
-In ~R() 4
-Done test_one {count = 3}
-In ~R() 3
+Done R assign {count = 2}
+Done test_one {count = 2}
In ~R() 2
After scope (setting global)
Before scope (begin)
Initializing a RefCount
In R initCopy {count = 1}
-In R initCopy {count = 2}
-Starting test_one {count = 3}
-In R.routine {count = 3}
-In R initCopy {count = 3}
-In test_two {count = 4}
-In ~R() 4
-Done test_one {count = 3}
-In ~R() 3
+Starting test_one {count = 2}
+In R.routine {count = 2}
+In test_two {count = 2}
+Done test_one {count = 2}
In ~R() 2
In ~R() 1
Destroying a RefCount
Index: test/nostdlib/recordpass7.chpl
===================================================================
--- test/nostdlib/recordpass7.chpl (revision 22894)
+++ test/nostdlib/recordpass7.chpl (working copy)
@@ -12,7 +12,7 @@
var k:int;
}
-proc rfun(x:R) {
+proc rfun(in x:R) {
x.j = 17;
}
Index: test/nostdlib/recordpass6.chpl
===================================================================
--- test/nostdlib/recordpass6.chpl (revision 22894)
+++ test/nostdlib/recordpass6.chpl (working copy)
@@ -12,7 +12,7 @@
var k:int;
}
-proc rfun(x:R):R {
+proc rfun(in x:R):R {
x.j = 17;
return x;
}
Index: test/functions/deitz/test_formal_copy1.chpl
===================================================================
--- test/functions/deitz/test_formal_copy1.chpl (revision 22894)
+++ test/functions/deitz/test_formal_copy1.chpl (working copy)
@@ -5,7 +5,7 @@
var gr : R;
-proc foo(lr : R) {
+proc foo(in lr : R) {
lr.x = 100;
writeln(lr);
writeln(gr);
Index: modules/internal/ChapelStandard.chpl
===================================================================
--- modules/internal/ChapelStandard.chpl (revision 22894)
+++ modules/internal/ChapelStandard.chpl (working copy)
@@ -32,6 +32,7 @@
use ChapelTaskTable;
use MemTracking;
use ChapelUtil;
+ use ChapelDynDispHack;
// Standard modules.
use Types;
Index: modules/internal/ChapelDynDispHack.chpl
===================================================================
--- modules/internal/ChapelDynDispHack.chpl (revision 0)
+++ modules/internal/ChapelDynDispHack.chpl (revision 0)
@@ -0,0 +1,14 @@
+pragma "no use ChapelStandard"
+module ChapelDynDispHack {
+ //
+ // This is an incredibly lame hack. If the test:
+ //
+ // test/classes/bradc/arrayInClass/arrayDomInClassRecord-simpler.chpl
+ //
+ // works with the following line removed then it sounds like we've
+ // moved to a better world than the one in which I committed this,
+ // and we can remove this check. See the comments in that test for
+ // more details.
+ //
+ assert (here.id >= 0);
+}
\ No newline at end of file
Index: modules/internal/ChapelBase.chpl
===================================================================
--- modules/internal/ChapelBase.chpl (revision 22894)
+++ modules/internal/ChapelBase.chpl (working copy)
@@ -1129,7 +1129,7 @@
// to be special-cased in functionResolution.cpp such that the inout
// does not actually result in temps.
//
- proc chpldev_refToString(inout arg) {
+ proc chpldev_refToString(ref arg) {
//
// print out the address of class references as well
Index: compiler/AST/symbol.cpp
===================================================================
--- compiler/AST/symbol.cpp (revision 22894)
+++ compiler/AST/symbol.cpp (working copy)
@@ -857,6 +857,7 @@
}
return (intent == INTENT_BLANK || intent == INTENT_CONST) &&
!isReferenceType(type) &&
+ !isSyncType(type) &&
!isRecordWrappedType(type) /* array, domain, distribution */;
}
Index: compiler/resolution/functionResolution.cpp
===================================================================
--- compiler/resolution/functionResolution.cpp (revision 22894)
+++ compiler/resolution/functionResolution.cpp (working copy)
@@ -283,7 +283,7 @@
static void resolveTupleExpand(CallExpr* call);
static void resolveSetMember(CallExpr* call);
static void resolveMove(CallExpr* call);
-static bool formalRequiresTemp(ArgSymbol* formal);
+static bool formalRequiresTemp(ArgSymbol* formal, FnSymbol* fn);
static void insertFormalTemps(FnSymbol* fn);
static void addLocalCopiesAndWritebacks(FnSymbol* fn, SymbolMap& formals2vars);
static Type* param_for_index_type(CallExpr* loop);
@@ -3262,64 +3262,38 @@
}
}
-// Returns true if the formal needs an internal temporary, false otherwise.
-static bool
-formalRequiresTemp(ArgSymbol* formal) {
-// We mostly just weed out the negative cases.
- // Look at intents. No temporaries for param, type or ref intents.
- if (formal->intent == INTENT_PARAM ||
- formal->intent == INTENT_TYPE ||
- formal->intent == INTENT_REF ||
- formal->intent == INTENT_CONST_REF ||
- (formal->intent == INTENT_BLANK &&
- (formal->type->symbol->hasFlag(FLAG_REF) ||
- isAtomicType(formal->type))))
- return false;
+static bool typeRequiresCopyForIn(Type* t) {
+ return (t->symbol->hasFlag(FLAG_ARRAY) ||
+ t->symbol->hasFlag(FLAG_DOMAIN) ||
+ isRecord(t));
+ // || t->symbol->hasFlag(FLAG_ITERATOR_CLASS));
+}
- // Some more obscure call-by-ref cases.
- if (formal->hasFlag(FLAG_IS_MEME) ||
- formal->hasFlag(FLAG_TYPE_VARIABLE) ||
- formal->hasFlag(FLAG_NO_FORMAL_TMP))
- return false;
-
- // The fLibraryCompile flag was added to support separate compilation.
- // Several code generation functions crash if it is not there.
- // This makes exported object arguments read-only which is not what we want.
- if (formal->hasFlag(FLAG_ARG_THIS)) {
- if (!fLibraryCompile)
- // So call-by-ref is cancelled if we are compiling an exported function.
- return false;
- }
-
- if (formal == toFnSymbol(formal->defPoint->parentSymbol)->_outer ||
- formal->instantiatedParam ||
- formal->type == dtMethodToken)
- return false;
-
- return true;
+// Returns true if the formal needs an internal temporary, false otherwise.
+static bool
+formalRequiresTemp(ArgSymbol* formal, FnSymbol* fn) {
+ /* These are passed by ref at the C level, so we need to make an
+ explicit copy in the function */
+ return (formal->intent == INTENT_OUT ||
+ formal->intent == INTENT_INOUT ||
+ /* The following cases also require a semantic copy, but we may
+ be able to rely on C's copy when passing the argument if we're
+ not inlining the function. */
+ ((formal->intent == INTENT_IN ||
+ formal->intent == INTENT_CONST_IN) &&
+ !fn->hasFlag(FLAG_INLINE) && typeRequiresCopyForIn(formal->type))
+ /* The following case reduces memory leaks, but I can't explain
+ why it'd be needed */
+ // || strcmp(formal->name, "followThis") == 0) {
+ );
}
static void
insertFormalTemps(FnSymbol* fn) {
- if (!strcmp(fn->name, "_init") ||
- !strcmp(fn->name, "_cast") ||
- fn->hasFlag(FLAG_AUTO_COPY_FN) ||
- fn->hasFlag(FLAG_AUTO_DESTROY_FN) ||
- fn->hasFlag(FLAG_INIT_COPY_FN) ||
- !strcmp(fn->name, "_getIterator") ||
- !strcmp(fn->name, "_getIteratorHelp") ||
- !strcmp(fn->name, "iteratorIndex") ||
- !strcmp(fn->name, "iteratorIndexHelp") ||
- !strcmp(fn->name, "=") ||
- !strcmp(fn->name, "_createFieldDefault") ||
- !strcmp(fn->name, "chpldev_refToString") ||
- fn->hasFlag(FLAG_ALLOW_REF) ||
- fn->hasFlag(FLAG_REF))
- return;
SymbolMap formals2vars;
for_formals(formal, fn) {
- if (formalRequiresTemp(formal)) {
+ if (formalRequiresTemp(formal, fn)) {
SET_LINENO(formal);
VarSymbol* tmp = newTemp(astr("_formal_tmp_", formal->name));
formals2vars.put(formal, tmp);
------------------------------------------------------------------------------
Learn Graph Databases - Download FREE O'Reilly Book
"Graph Databases" is the definitive new guide to graph databases and their
applications. Written by three acclaimed leaders in the field,
this first edition is now available. Download your free book today!
http://p.sf.net/sfu/13534_NeoTech
_______________________________________________
Chapel-developers mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/chapel-developers
