[GC] Precompute: Fix ref.eq comparisons of structs with nested effects

[kripken]

In the testcase here, the nested tee make the outer struct not precomputable
when we care about effects (tee is an effect). But we could precompute it
otherwise, and that ended up causing confusion, with no caching being done
of the canonical allocation for it, meaning each time we scanned it we saw
another struct, different in identity.

To fix this, implement the TODO to cache in both modes. I thought that would
be useful only for speed, but turns out it is necessary for correctness. Luckily
doing so is quite simple.

[aheejin]

Some basic questions:

• Why is this case a problem for correctness and not only for speed?
• When do we preserve side effects, and when do we not? Scanning the code, it looks we preserve side effects when we actually try to replace a value (for correctness) and don't preserve it when just evaluating whether something is a constant. But if we have to preserve side effects when replacing anyway, why do we need two separate modes? For speed?

[kripken]

The correctness issue is because we need the caching (as I forgot). The caching ensures that each struct.new is represented by a single allocated object in the interpreter. If we don't cache those values, the same struct.new will look like a different allocation if we compute it twice (and do not cache). And when it looks different, ref.eq will be wrong, as the testcase shows.

Yes, the two modes are for when we replace and when we don't. When we want to replace an expression, we can't ignore side effects like local.tee (if we replaced it with a precomputed constant, the tee's effect would vanish). But, when we are just propagating values around, we want to see the value of an expression (so if it evaluates to a precomputed constant, we can propagate that; and it's fine if there are side effects, we are leaving them alone).

[aheejin]

I can understand that if we ignore side effect (like tee) and replace some expression with a constant, the tee would vanish. But how do the withEffects and withoutEffects maps convey that? It's value is just a GCData, which I presume a constant that an expression boils down to, and does not seem to contain info on whether a tee is set or not.

[kripken]

It is just that we need two separate maps so we don't "mix" things. Imagine we compute

(struct.new (local.tee ))

If we compute it when we don't care about effects, we can store PTR in the cache (the pointer to the allocation that represents it, in the interpreter). Then, later, if we do care about effects, if we read from that cache, we'd get PTR, but that is wrong: if we ignored the cache, we'd get the right result (when effects matter), which is to stop at the local.tee because of its effects.

Basically, the two modes can get different results for computing the same expression. To preserve the property that the cache returns the value we would have computed anyhow, we need one cache for each.

[aheejin]

It is just that we need two separate maps so we don't "mix" things. Imagine we compute

(struct.new (local.tee ))

Were you gonna put something in (local.tee)?

If we compute it when we don't care about effects, we can store PTR in the cache (the pointer to the allocation that represents it, in the interpreter). Then, later, if we do care about effects, if we read from that cache, we'd get PTR, but that is wrong: if we ignored the cache, we'd get the right result (when effects matter), which is to stop at the local.tee because of its effects.

Basically, the two modes can get different results for computing the same expression. To preserve the property that the cache returns the value we would have computed anyhow, we need one cache for each.

So in this case, if there is a tee, withoutEffects will give you a constant value, but withEffects wouldn't, because it cannot be replaced by that single constant, right?

[aheejin]

If #7857 (comment) is a correct interpretation of your intention, LGTM. (Please let me know if not 😅)

[kripken]

Yes, and yes, exactly.

[tlively]

Any guesses as to why the fuzzer did not catch this? Do we need to emit more ref.eq?

[kripken]

I'm really not sure how this was missed by the fuzzer. Still thinking about it / concerned...