[Custom Descriptors] Fix CFP on an exact ref.get_desc

[kripken]

ref.get_desc inherits its input ref's exactness: When called on an
exact 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_desc returned an exact type, but if we consider values
from 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 now
struct.get of an exact type will ignore subtypes.

[tlively]

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.

[kripken]

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).

[tlively]

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.

[kripken]

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.

[tlively]

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.

[kripken]

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.

[tlively]

What purpose does the funcref field serve? Can it be removed?

[kripken]

Good point, removed.

[tlively]

Looks like it's still there. Forgot to push?

[kripken]

Oops, yes, pushed now.

[tlively]

Maybe let's land the previous version to unbreak the calcworker build, then I can take a look at refactoring if you'd like.

[tlively]

See #7889 for the alternative I had in mind.

[kripken]

Rereading this with fresh eyes, I think the current PR is the way to go. The later commit's diff is mostly undoing the first part of the PR - looking at the total state of the PR now, this is just a few lines, and trivial.

So PTAL at the current state. (Your alternative also looks promising but it is doing a lot more, so I suggest we land this first.)

[tlively]

This does not appear to be correct. Here's a test case:

(module
  ;; CHECK:      (type $A (struct (field (mut i32))))
  (type $A (struct (field (mut i32))))

  ;; CHECK:      (type $1 (func))

  ;; CHECK:      (func $test (type $1)
  ;; CHECK-NEXT:  (local $A.exact (ref (exact $A)))
  ;; CHECK-NEXT:  (local.set $A.exact
  ;; CHECK-NEXT:   (struct.new $A
  ;; CHECK-NEXT:    (i32.const 10)
  ;; CHECK-NEXT:   )
  ;; CHECK-NEXT:  )
  ;; CHECK-NEXT:  (struct.set $A 0
  ;; CHECK-NEXT:   (local.get $A.exact)
  ;; CHECK-NEXT:   (i32.const 20)
  ;; CHECK-NEXT:  )
  ;; CHECK-NEXT:  (drop
  ;; CHECK-NEXT:   (block (result i32)
  ;; CHECK-NEXT:    (drop
  ;; CHECK-NEXT:     (ref.as_non_null
  ;; CHECK-NEXT:      (local.get $A.exact)
  ;; CHECK-NEXT:     )
  ;; CHECK-NEXT:    )
  ;; CHECK-NEXT:    (i32.const 10)
  ;; CHECK-NEXT:   )
  ;; CHECK-NEXT:  )
  ;; CHECK-NEXT: )
  (func $test
    (local $A.exact (ref (exact $A)))
    (local.set $A.exact
      (struct.new $A
        (i32.const 10)
      )
    )
    ;; This set prevents us from optimizing. We should not be confused by the
    ;; exactness of the ref.
    (struct.set $A 0
      (local.get $A.exact)
      (i32.const 20)
    )
    (drop
      (struct.get $A 0
        (local.get $A.exact)
      )
    )
  )
)

[kripken]

Good catch, I guess I was thinking only of descriptors...

Fixed in the last commit. We just need to check all the conditions that allow us to read structNews only (exact ref + immutability)

[kripken]

Good news, the fuzzer found this over the weekend. Took 100K iterations though...