Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Inliner brings non-dominating uses into environment #36899

Closed
aartbik opened this issue May 8, 2019 · 2 comments
Closed

Inliner brings non-dominating uses into environment #36899

aartbik opened this issue May 8, 2019 · 2 comments
Assignees
@aartbik
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends.

Comments

@aartbik
Copy link
Contributor

aartbik commented May 8, 2019

For example, we get the following situation, where the "Redefinition" in B11 actually appear in the environment of the "goto B11" in B16, even though B11 does (obviously) not dominate B16.

B16[target]:2 has-env specu repr=tagged
    goto:2 B11 has-env specu repr=tagged

B11 pre#=9 post#=2
dom=B10
doms(#0)=
succ=B18,
preds=B13,B16,
B11[join]:2 pred(B13, B16) has-env specu repr=tagged
    v33 <- Redefinition(v2) T{_StreamSinkImpl} specu repr=tagged tp=T{_StreamSinkImpl} U[,]
    v29 <- LoadField(v33 T{_StreamSinkImpl<T>} . _doneCompleter@19463476 {final}) T{_AsyncCompleter} specu repr=tagged tp=T{_AsyncCompleter} U[,]
    v31 <- LoadField(v29 . future {final}) T{_Future} specu repr=tagged tp=T{_Future} U[,]
    goto:18 B18 has-env specu repr=tagged

@mraleph
@aartbik aartbik added the area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. label May 8, 2019
@aartbik aartbik self-assigned this May 8, 2019
dart-bot pushed a commit that referenced this issue May 13, 2019
@aartbik
[dart/vm] enhanced graph checker
b0727bd 
Rationale:
Refactored existing verify uses (with some overlap and some
additional checks) into the graph checker. Also added more
verification code and repaired some inconsistencies in IR
found by new checker. However, some checks are disabled with
a TODO, since the IR does not currently meet all the stricter
assumptions. Fixes will follow.

#36893
#36894
#36895
#36899

Change-Id: Ic0395208da38ecb6fc8ca2551efe819e6458a731
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/101922
Commit-Queue: Aart Bik <[email protected]>
Reviewed-by: Alexander Markov <[email protected]>
dart-bot pushed a commit that referenced this issue May 15, 2019
@aartbik
[dart/vm] Fix inconsistencies in intermediate representation
57c970d 
Rationale:
Stricter graph checking found some dangling pointers
and missing updates. This CL fixes the omissions
and removes some bail-outs in the graph checker itself,
making the checker stricter and more consistent. Also
adds flag to control verification (avoid excessive runtimes
for large programs in general, but allows testing even
these through command line option).

#36893
#36894
#36895
#36899

Change-Id: If4357cb897484ddfdb60722525092198771ec90a
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/102420
Reviewed-by: Aart Bik <[email protected]>
Commit-Queue: Aart Bik <[email protected]>
@aartbik
Copy link
Contributor Author

aartbik commented Jul 31, 2019 •
edited
Loading

Pending CL https://dart-review.googlesource.com/c/sdk/+/111502 fixes the redefinition dominance issue. After this we still have one violation in the def-use check that needs to be fixed.

The following assert assignable checks have env uses that are not dominated by the subsequent push arguments.

dart:collection__CustomHashSet@3220832__equals@3220832

v70 <- LoadField(v64 . :type_arguments {final}) T{TypeArguments} specu repr=tagged tp=T{TypeArguments} #uses=1 #env-uses=1
AssertAssignable:12(v66, E, '', instantiator_type_args(v70), function_type_args(v59)) T{E?} env={ v64, v60, v68, v59, v59, a0, v66, v66, v70, v59 } env specu can-deopt repr=tagged tp=T{E?} #uses=0 #env-uses=0
v72 <- LoadField(v64 . :type_arguments {final}) T{TypeArguments} specu repr=tagged tp=T{TypeArguments} #uses=1 #env-uses=1
AssertAssignable:14(v68, E, '', instantiator_type_args(v72), function_type_args(v59)) T{E?} env={ v60, v60, v60, v59, v59, a0, a1, v68, v68, v72, v59 } env specu can-deopt repr=tagged tp=T{E?} #uses=0 #env-uses=0
v76 <- LoadField(v64 . _equality@3220832 {final}) T{(E, E) => bool?} specu repr=tagged tp=T{(E, E) => bool?} #uses=1 #env-uses=0
PushArgument(v76) specu repr=tagged tp=T{*?} #uses=0 #env-uses=3
PushArgument(v66 T{E?}) specu repr=tagged tp=T{*?} #uses=0 #env-uses=2
PushArgument(v68 T{E?}) specu repr=tagged tp=T{*?} #uses=0 #env-uses=1

dart-bot pushed a commit that referenced this issue Aug 1, 2019
@aartbik
[dart/vm] Fix dominance violation
150e8f8 
Rationale:
The flow graph checker still had a few ugly exceptions
due to dominance violation after inlining. This CL fixes
the violation, removes the exceptions from the flow graph
checker, and also adds a new utility that can be used in
some other places.

#36899

Change-Id: I42766af22a3696fe050b0e5c4a2dfe930933fc40
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/111502
Reviewed-by: Martin Kustermann <[email protected]>
Commit-Queue: Aart Bik <[email protected]>
@aartbik
Copy link
Contributor Author

aartbik commented Aug 2, 2019 •
edited
Loading

The remaining dominance violation is introduced in AotCallSpecializer::TryExpandCallThroughGetter() which moves the PushArguments closer to a newly introduced call. Trying to restore the environment, keeping the PushArguments in the right order, and evaluating the actual parameter values in the right order too all at once seems an overly complex task. So let's keep a very special case in the flow graph checker for this (which can be removed if we ever decide to get rid of PushArguments altogether).

dart-bot pushed a commit that referenced this issue Aug 2, 2019
@aartbik
[dart/vm] Check SSA def-use dominance only
19085e3 
Rationale:
PushArguments violate some def-use dominance relations
after inlining. Fixing this seems not worth the engineering
effort. Rather we wait until we remove PushArguments
altogether (the optimizing compiler has no need for them).

#36899

Change-Id: I9265420ae8ec4981fe7170b9a1869dd56ae323cc
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/111880
Reviewed-by: Martin Kustermann <[email protected]>
Commit-Queue: Aart Bik <[email protected]>
@aartbik aartbik closed this as completed Aug 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@aartbik aartbik

Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@aartbik