Skip to content

[NFC] Improve OnceReduction comment - #6068

Merged
kripken merged 5 commits into
mainfrom
moar.three
Oct 31, 2023
Merged

[NFC] Improve OnceReduction comment#6068
kripken merged 5 commits into
mainfrom
moar.three

Conversation

@kripken

Copy link
Copy Markdown
Member

Followup to #6061 in which we only optimized the case of a "once" function immediately
calling another and doing nothing else. It is ok to do more things afterwards, so long as
we do nothing else before, by the same logic as already mentioned in the pass (but
clarified as to the meaning of "called" - all we need is for the function to have been
"entered" in the proof there, IIANM).

@gkdn am I missing something?

@gkdn

gkdn commented Oct 31, 2023

Copy link
Copy Markdown
Contributor

Are you proposing; "we can still remove the condition, if the once function does more than calling another once function"?

@kripken

Copy link
Copy Markdown
MemberAuthor

Yes, exactly. Concretely:

(func$once
(if
(global.get$once)
(return)
)
(global.set$once (i32.const1))
;; Call another once function. This lets us remove the if and global.set.
(call$once.1)
;; Do other stuff. This does not stop us, since this PR.
(call$anything)
)

@gkdn

gkdn commented Oct 31, 2023

Copy link
Copy Markdown
Contributor

if anything calls back $once, we would no longer have the guard so anything would be called again.

@kripken

Copy link
Copy Markdown
MemberAuthor

Oh right... how silly of me!

I'll update this PR to fix the proof text and remove the change.

@kripken
kripken requested a review from tlivelyOctober 31, 2023 21:07
@kripkenkripken changed the title OnceReduction: Optimize "once" functions calling others even if they do other things later[NFC] Improve OnceReduction commentOct 31, 2023
@kripken

Copy link
Copy Markdown
MemberAuthor

This PR now only adds a comment to explain why we can only optimize in the case that we do.

@kripken

Copy link
Copy Markdown
MemberAuthor

This weird ASan error seems like a random flake... it happens once every few PRs now. Very strange. I wish we had a way to ssh into the CI here to investigate...

@kripken
kripken merged commit c82627e into mainOct 31, 2023
@kripken
kripken deleted the moar.three branch October 31, 2023 22:47
radekdoulik pushed a commit to dotnet/binaryen that referenced this pull request Jul 12, 2024
Sign up for freeto 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

@kripken@gkdn@tlively