-
Notifications
You must be signed in to change notification settings - Fork 831
ConstantFieldPropagation: Add a variation that picks between 2 values using RefTest #6692
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.
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
Conversation
tlively
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here are comments on the code; I still need to review the tests.
| // precise, |rawNewInfos|, which tracks the values written to struct.news, | ||
| // where we know the type exactly (unlike with a struct.set). But for that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the rest of this pass analyze mutable fields at all? If not, then the fact that we're only looking at constant fields might be a simpler explanation for why we only look at immutable fields here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the rest of the pass does analyze mutable fields. We inspect each struct.set to see if it writes the same constant value.
| } | ||
|
|
||
| // If we filled slot 1 then we must have filled slot 0 before. | ||
| assert(values[1].used() ? values[0].used() : true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| assert(values[1].used() ? values[0].used() : true); | |
| assert(!values[1].used() || values[0].used()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yours is shorter, but mine matches the text, so I think it might be clearer? That is, the text says "if X then necessarily Y" and the code is "X ? Y : true".
Another option might be
if (X) {
assert(Y);
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know, I think having a bool literal in a ternary is more surprising than a simple boolean formula.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough, rewritten.
The real problem is that programming languages lack the math arrow operator...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be nice!
| // We did not see two constant values (we might have seen just one, or | ||
| // even no constant values at all). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose the case where we've seen just one value will be handled separately? Can we guarantee that we don't do all this extra work when it will already be handled separately?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rest of the pass handles the case of just one value, so we should not get here in that case (we'd have optimized that one value). Perhaps we can even assert num != 1 here. But that won't save the work we did up to now in this function, since we need to see if num == 2 exactly (not 0, nor 3).
| // optimization if the type we test on is closed/final, since ref.test on a | ||
| // final type can be fairly fast (perhaps constant time). We therefore look |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have we experimentally determined that doing this for non-final types is a bad idea? Given the complexity of checking for non-overlapping final types below, the more general solution might even be simpler to implement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't measure it, I was relying on what I have heard about the cost of ref.test in general. I'll ask for more clarification.
It would be more complex, however, to do the general thing, I had that code earlier at some point. We need to compute two LUBs and see one is nested in the other, iirc, something like that.
test/lit/passes/cfp-reftest.wast
Outdated
| ;; CHECK-NEXT: ) | ||
| ;; CHECK-NEXT: ) | ||
| (func $create | ||
| (local $keepalive (ref $substruct)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't even need a local to keep it alive because the type definition will already keep it alive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, good point, in this case there is indeed a type def that uses it.
| ) | ||
| ) | ||
| ) | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could also test with two top-level structs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, added.
CFP focuses on finding when a field always contains a constant, and then replaces
a
struct.getwith that constant. If we find there are two constant values, then in somecases we can still optimize, if we have a way to pick between them. All we have is the
struct.getand its reference, so we must use aref.test:This is valid if, of all the subtypes of
$T, those that pass the test haveconstant1 in that field, and those that fail the test have constant2. For
example, a simple case is where
$Thas two subtypes,$Tis never createditself, and each of the two subtypes has a different constant value.
This is a somewhat risky operation, as
ref.testis not necessarily cheap.To mitigate that, this is a new pass,
--cfp-reftestthat is not run bydefault, and also we only optimize when we can use a
ref.teston whatwe think will be a final type (because
ref.teston a final type can befaster in VMs).