Improve CFP by removing rawNewInfos #7892
Conversation
Key collected struct information on both heap type and exactness, allowing queries for exact types to return more precise results than queries on the corresponding inexact types. Use this to fix a bug in CFP where it failed to take into account exactness and would unnecessarily and incorrectly emit a select between two values of different types where a single exact type was expected. Also update GTO to propagate to exact types even though it does not take advantage of them. This is necessary because the FieldScanner now collects information for exact and inexact types separately and they need to be combined.
CFP takes advantage of exact type information, but it currently does so only for immutable fields. It is also unnecessarily conservative about how it propagates type information so that sets to a type inhibit optimizations of its sibling types, even though those sets cannot possibly affect the siblings. Add tests for these cases to demonstrate the benefit of follow-on PRs that will fix these issues.
For the purposes of CFP, there's nothing fundamentally different between a set on an exact reference and a value set by allocation. CFP's use of the allocation values without considering all exact sets was therefore an unnecessary complication that restricted CFP's optimizing power. Expand optimizeUsingRefTest to optimize mutable fields, including those that have been set, by using the full available information instead of just the allocation values. Handle copies more judiciously by propagating once to find copied values and then propagate again while taking those copied values into account. This scheme can be extended in the future to precisely handle copies between different fields and types as well. Also optimize siblings better by propagating first down and then up rather than propagating in both directions at once. This avoid unnecessarily propagating set values to siblings.
Co-authored-by: Alon Zakai <azakai@google.com>
|
cc @kripken in case you missed this one. |
|
Wait, I thought this depended on the other large PR for CFP - does it not? (I was reading them in order of issue number, as we said) |
|
Oh sorry, I don't remember why this one is out of order. This is the next one to be merged, though. (You can tell because its target branch is |
| // that is allowed. | ||
| if (!info.isConstant()) { | ||
| if (refTest) { | ||
| if (refTest && !ref->type.isExact()) { |
There was a problem hiding this comment.
Maybe add a comment here, e.g. Note that if the reference is exact, we never need to use a ref.test, as we will not have two types to select between
| PCVStructValuesMap refTestInfos; | ||
| propagator.propagateToSubTypes(combinedSetInfos); | ||
| if (refTest) { | ||
| refTestInfos = combinedSetInfos; |
There was a problem hiding this comment.
It looks like the refTestInfos have more propagation than before - before we just used the raw new infos, which are exact? That was valid because we considered only immutable fields, but looks like this PR removes that - the description mentions that, but I don't quite see why it doesn't regress? Does the propagation not lose something?
There was a problem hiding this comment.
Rather than regressing the optimization, this change improves the optimization. Previously we considered only the value exactly set by allocations and made this safe by considering only immutable fields. Now we consider all values set anywhere, which makes it safe to optimize mutable fields as well. And for the immutable fields that were being optimized before, there cannot possibly be any sets besides those from the allocation, so their optimizations cannot regress when we look at all the sets.
There was a problem hiding this comment.
Oh, I see... we do propagate those allocations to subtypes, but as exact, they don't have any?
|
Oh, right... maybe the 7899/7889 thing confused me... |
test/lit/passes/cfp-reftest.wast
Outdated
| ;; CHECK-NEXT: ) | ||
| ;; CHECK-NEXT: ) | ||
| (func $get (param $struct (ref null $struct)) (result i32) | ||
| ;; We cannot optimize here. |
There was a problem hiding this comment.
Indeed, this is an old, stale comment that only appears as an addition in the diff because of the preceding changed test expectations.
test/lit/passes/cfp-reftest.wast
Outdated
| ;; CHECK-NEXT: ) | ||
| ;; CHECK-NEXT: ) | ||
| (func $get (param $struct (ref null $struct)) (result i32) | ||
| ;; We cannot optimize here. |
There was a problem hiding this comment.
ditto. Or am I misunderstanding what "optimize" means?
There was a problem hiding this comment.
Nope, you're right. Removed the comments.
For the purposes of CFP, there's nothing fundamentally different between
a set on an exact reference and a value set by allocation. CFP's use of
the allocation values without considering all exact sets was therefore
an unnecessary complication that restricted CFP's optimizing power.
Expand optimizeUsingRefTest to optimize mutable fields, including those
that have been set, by using the full available information instead of
just the allocation values.
Handle copies more judiciously by propagating once to find copied values
and then propagate again while taking those copied values into account.
This scheme can be extended in the future to precisely handle copies
between different fields and types as well.
Also optimize siblings better by propagating first down and then up
rather than propagating in both directions at once. This avoid
unnecessarily propagating set values to siblings.