[Custom Descriptors] Fix CFP on an exact ref.get_desc #7886
Conversation
| // We found exactly one value. This can happen because we consider | ||
| // subtyping more carefully than the non-reftest logic (specifically, we | ||
| // notice exact types). Optimize to the single possible value. | ||
| optimizeSingleValue(values[0].constant, refHeapType, curr, ref); |
There was a problem hiding this comment.
Can we not make the original analysis more precise by taking exactness into account when propagating information? That seems like it would be more robust than detecting and fixing up the imprecision later.
There was a problem hiding this comment.
Note that we aren't fixing it up later: We are reading from rawNewInfos just below, that is, we are not reading propagated info, but info straight from struct.news, which is precise.
Improving the propagation could be good as well, though it would not help this part of the optimization (since it already looks at precise data).
There was a problem hiding this comment.
I mean that we should never even call optimizeUsingRefTest if there is only a single constant value when you take exactness into account. It would be best if the propagated info used by optimizeRead already accounted for exactness. Then we wouldn't have to complicate optimizeUsingRefTest by having it sometimes optimize not using RefTest.
There was a problem hiding this comment.
I see your point. However, I think the best course of action is to refactor this code so that the reftest code is used by both paths. That is, the reftest code has precise analysis already - we can just use that, without (harder) work to change the propagation. (And then we can rename it "the general path", with a flag "allow reftest")
Put another way, we propagate for the inexact case, and keep the raw data for the exact case. And we have a function that uses that data correctly right now, reftest. Changing the propagation would be more work.
I'd like to leave that as a followup, though, to keep this PR simple.
There was a problem hiding this comment.
I don't want to delay getting this fix in, but I really do think making the propagation more precise is the right fix. Then no other future pass that also uses struct-utils.h will have to special-case exactness to avoid missing an optimization.
There was a problem hiding this comment.
I'm not opposed to precise propagation, but it's a separate issue from this PR. Propagation is done so that we can easily notice the effects of subtypes. But when we have exact info, we can just read that info, that we do have it here - rawNewInfos as I mentioned. So the only question is where to read that info. The simplest thing seemed to be to use it in the reftest logic, so I started with that.
But, it might be clearer to just use exactness earlier, not just in the reftest logic. Might be faster too. I pushed a commit with that now. It's late on Friday so I'm not sure I got it right... I'll check on Monday.
| ;; CHECK-NEXT: (type $super (sub (descriptor $super.desc (struct)))) | ||
| (type $super (sub (descriptor $super.desc (struct)))) | ||
| ;; CHECK: (type $super.desc (sub (describes $super (struct (field (ref (exact $func))))))) | ||
| (type $super.desc (sub (describes $super (struct (field (ref (exact $func))))))) |
There was a problem hiding this comment.
What purpose does the funcref field serve? Can it be removed?
There was a problem hiding this comment.
Looks like it's still there. Forgot to push?
kripken
force-pushed
the
cfp.reftest.desc.exact
branch
from
adaffba to
e42cbb0
Compare
|
Maybe let's land the previous version to unbreak the calcworker build, then I can take a look at refactoring if you'd like. |
|
See #7889 for the alternative I had in mind. |
ref.get_descinherits its input ref's exactness: When called on anexact ref, it returns the exact descriptor type. CFP was not aware of
that, and thought any subtype can appear as well.
That was not only a missed optimization, but also a validation bug:
ref.get_descreturned an exact type, but if we consider valuesfrom subtypes, we may emit a select over two values that is no
longer exact.
This also helps optimize
struct.get, as the logic is shared, so nowstruct.getof an exact type will ignore subtypes.