Skip to content

Conversation

@kripken
Copy link
Member

@kripken kripken commented Feb 1, 2023

If traps can happen, then we can't always remove a trap on null
on the ref input to struct.set, since it has two children,

(struct.set
  (ref.as_non_null X)
  (call $foo))

Removing the ref.as would not prevent a trap, as the struct.set
will trap, but it does move the trap to after the call which is bad.

@kripken kripken requested a review from tlively February 1, 2023 23:52
Comment on lines +1509 to +1510
if (EffectAnalyzer(options, *getModule(), child)
.hasSideEffects()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be sufficient to check for control flow side effects here? Updating a local certainly won't matter, for instance. I guess updating a global might matter, though?

But I guess you'll say that side effects are rare enough that it doesn't really matter and we should do the simplest thing here 👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, we could do a little better here. We can also ignore traps, since traps are ok to reorder. I added a TODO as it seems low priority, and would probably require some tinkering in effects.h.

@kripken kripken enabled auto-merge (squash) February 2, 2023 20:51
@kripken kripken merged commit d3f2df3 into main Feb 2, 2023
@kripken kripken deleted the struct.set.ref.cast.ordering branch February 2, 2023 21:13
kripken added a commit that referenced this pull request Feb 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants