All: I will review this patch with an eye to the change in memory usage.
Even without opening the patch I can say that changes in memory usage behavior in iterators is not a show-stopper. Based on recent work I have been doing in that area, it is clear that memory management in iterators is not "correct" in the UMM sense. As long as the affected tests can be executed under valgrind without reporting memory usage errors, the change should be deemed acceptable. I'll provide feedback on the patch itself by noon on Monday. THH ________________________________________ From: Brad Chamberlain [[email protected]] Sent: Saturday, March 15, 2014 3:59 PM To: Chapel Sourceforge Developers List Subject: [Chapel-developers] [patch] extraneous formal temp elimination 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. ------------------------------------------------------------------------------ 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
