Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Merge pull request #5193 from vasslitvinov/newMemDesc-on-Symbols,-not…
…-names Create same number of mem descs w/ and w/o --devel MOTIVATION ---------- I found the number of AST nodes created with and without --devel to differ. Here is what I believe was the cause. callChplHereAlloc() invoked newMemDesc() passing it the name of the type being allocated. newMemDesc() created a new memory descriptor for each new name that it got. That involved new_IntSymbol() and so a new VarSymbol + new DefExpr. When compiling with --no-devel, type names for domains/arrays are made nicer, e.g.: [CSRDom(2,int(64),domain(2,int(64),false))] int(64) Cf. with --devel they are more verbose, e.g.: BaseSparseArr(int(64),2,int(64),CSRDom(2,int(64),_domain(DefaultRectangularDom(2,int(64),false)))) BaseSparseArrImpl(int(64),2,int(64),CSRDom(2,int(64),_domain(DefaultRectangularDom(2,int(64),false)))) CSRArr(int(64),2,int(64),CSRDom(2,int(64),_domain(DefaultRectangularDom(2,int(64),false)))) BTW they come from the following locations, respectively: ChapelDistribution.chpl:689 ChapelDistribution.chpl:711 LayoutCSR.chpl:425 when compiling $CHPL_HOME/test/users/engin/sparse_bulk_dist/sparseBulk.chpl -ssparseLayoutType=CSR In a couple of cases, such as this, different types have the same (nicer) name with --no-devel whereas different names with --devel. So the latter caused extra new_IntSymbol() and so extra two nodes compared to the former. WITH THIS CHANGE ---------------- newMemDesc now accepts and hashes on the Type* to be allocated, so distinct types always get distinct memory descriptors regardless of --devel or not. As a result, in the above compilation the number of AST nodes matches between --devel and --no-devel until insertLineNumbers(). insertLineNumbers and codegen still add more nodes with --no-devel than with --devel. For the above compilation I got ~13k extra in insertLineNumbers and another ~1.1k in codegen. (I did not investigate these.) Interestingly, all calls to newMemDesc resulting in new descriptors come from resolution - except for one. It happens during scopeResolve calling callChplHereAlloc(fn->_this->type) (was callChplHereAlloc(fn->_this)) from within build_constructor(AggregateType* ct) invoked on the generic _ref type. The corresponding descriptor was associated with "this", now with "_ref". I did not investigate what becomes of this allocation when the constructor for _ref is instantiated, if it ever is. (I did not see any "this" descriptors from nightly memleaks testing.) This seems to be the first call to newMemDesc, so the memory descriptor is 0. IMPLEMENTATION -------------- As suggested by Michael, I made newMemDesc() create separate descriptor numbers based on the type, rather than its name. * This required a deviation from name-centric memory descriptors. In a few cases newMemDesc() is invoked on char* literals e.g. "global heap-converted data", "bundled args", etc. Prior to this change, there was a 1:1 between the name and the descriptor number. Now the same type name can have several descriptors if multiple Types have the same name. See the above example. newMemDesc() still has an implementation that accepts strings. It uses a separate hash table and issues distinct descriptors from those on Types. Michael and I discussed merging the two hash tables into one, however did not find an easy solution, and it did not seem worth it to me to try to merge them. * To implement that, I created a separate newMemDesc() and memDescsMap that operate on Types. Note that memDescsNodeMap hashes on Type ID, not the Type* pointer. This is to guard against the case where a Type is removed from tree and its pointer is reused. Even though such a case is unlikely. Michael has reservations about it, perhaps based on the rest of the compiler using node pointer rather than node ID for hashing in cases like this. I have been bitten once by pointer-based hashing when the node used as the key got deallocated then another node got allocated at the same address. The Map gave me an incorrect node as a result. Given that I am unaware of the policy that Type nodes are never deleted (even though in reality that may well be the case), I am being extra cautious and using IDs for hashing. However, we can revert to hashing on pointers if desired. MORE MEMORY DESCRIPTORS NOW - esp. for _ic_ types ------------------------------------------------- The names associated with memory descriptors in the compiled program's output - those are still always char* - are visible e.g. when running with --memLeaks. This is most prominent for iterator classes, they are named _ic_.... Now we create separate descriptors for ICs corresponding to serial, leader, follower, and standalone iterators (when applicable), whereas before they were collapsed into one. I left it this way encouraged by Brad. We can still collapse them by dispatching calls on IC types to the string-based implementation of newMemDesc. Of we can be fancy and extend the names for memory descriptors of IC types with "(leader)", "(follower)", etc. So now we may be observing multiple lines with the same descriptor name instead of a single line with combined counts. I did not come up with an example illustrating that. The test discussed above leaks those arrays, however it does not illustrate. We might see something in the nightly memleak testing where we do leak _ic_ things occasionally. WHILE THERE ----------- * I modified callChplHereAlloc() to always accept Type* instead of Symbol*, thus avoiding Type*->Symbol*->Type* conversion. The only place where this was non-trivial was replacement: callChplHereAlloc(fn->_this) --> callChplHereAlloc(fn->_this->type) in build_constructor(AggregateType* ct) (See above for a remark on this.) * Removed a couple of unnecessary lines from callChplHereAlloc(). * Removed an unnecessary SET_LINENO() call from scopeResolve(). * I considered removing the optional 'md' formal in callChplHereAlloc() because it is currently unused. I chose to leave it there just in case and for symmetry with insertChplHereAlloc. r: @mppf
- Loading branch information
