Add missing parent pointers in CopyingVisitor

Add missing parent pointers in CopyingVisitor

R=asgerf@google.com

Committed: https://github.com/dart-lang/sdk/commit/7778f54141f11ad3856734365d85f2646e35e754

[Siggi Cherem (dart-lang), 2016-01-22 20:56:41]

sigmund@google.com changed reviewers:
+ asgerf@google.com, kmillikin@google.com

[asgerf, 2016-01-22 21:12:36]

Nice catch. LGTM

[Siggi Cherem (dart-lang), 2016-01-22 21:40:57]

Description was changed from

==========
Add missing parent pointers in CopyingVisitor
==========

to

==========
Add missing parent pointers in CopyingVisitor

R=asgerf@google.com

Committed:
https://github.com/dart-lang/sdk/commit/7778f54141f11ad3856734365d85f2646e35e754
==========

[Siggi Cherem (dart-lang), 2016-01-22 21:40:58]

Committed patchset #1 (id:1) manually as
7778f54141f11ad3856734365d85f2646e35e754 (presubmit successful).

[Kevin Millikin (Google), 2016-01-25 10:15:42]

Why was this change necessary?  The call to ParentVisitor.setParents in
InliningCache.get should be enough.

Parent pointers are 'derived' and that's why I implemented the cache so it
didn't worry about maintaining them when it wasn't necessary, i.e., when terms
are in the cache.

Future work might involve a more compact serialized representation of terms, in
which case they probably won't have parent pointers anyway.

[Siggi Cherem (dart-lang), 2016-02-04 22:22:54]

On 2016/01/25 10:15:42, Kevin Millikin (Google) wrote:
> Why was this change necessary?  The call to ParentVisitor.setParents in
> InliningCache.get should be enough.
> 
> Parent pointers are 'derived' and that's why I implemented the cache so it
> didn't worry about maintaining them when it wasn't necessary, i.e., when terms
> are in the cache.
> 
> Future work might involve a more compact serialized representation of terms,
in
> which case they probably won't have parent pointers anyway.

Ah -- do you mean that to do an actual copy of an IR term I should call the
ParentVisitor after the CopingVisitor?

If so, then this change might not be necessary (although it might be worth
looking at whether it makes a difference in compile-time if we do this all at
once in one visitor or if the parentvisitor pass adds much to the compile-time
cost).

For context: I came across this trying to profile a phase. I was repeatedly
cloning and applying a phase. To clone the IR, I used the CopingVisitor, but I
didn't know about the ParentVisitor. I was just assuming that the CopingVisitor
would preserve the invariant of having parent pointers in place, but then run
into problems when running the phase afterwards.