Skip to content

Conversation

rluble
Copy link
Contributor

@rluble rluble commented May 6, 2024

Constants that need to be hoisted sometimes are initialized by calling getters of other constants that need to be hoisted. These getters are non-trivial, e.g.

 (func $getConst1_<once>_@X (result (ref null $A)) (block (result (ref null $A)) (if (i32.eqz (ref.is_null (global.get $$const1@X))) (then (return (global.get $$const1@X)) ) ) (global.set $$const1@X (struct.new $A (i32.const 2))) (global.get $$const1@X) ) (func $getConst2_<once>_@X (result (ref null $A)) (block (result (ref null $A)) (if (i32.eqz (ref.is_null (global.get $$const2@X))) (then (return (global.get $$const2@X)) ) ) (global.set $$const2@X .... expression with (call $getConst1_<once>_@X) ....)) (global.get $$const2@X) ) 

and can only be simplified after the constants they initialize are hoisted. After the constant is hoisted the getter can be inlined and constants that depend on it for their initialization can now be hoisted.

Before this pass, inilining would happen after the pass was run by a subsquent run of the inliner (likely as part of -O3), requiring as many runs of this pass, interleaved with the inliner, as the depth in the call sequence.

By having a simpler inliner run as part of the loop in this pass, the pass becomes more effective and more independent of the call depths.

@gkdn
Copy link
Contributor

gkdn commented May 7, 2024

_ No description provided. _

Let's put some detailed description of what and why we are doing for future reference.

@rluble
Copy link
Contributor Author

rluble commented May 8, 2024

I think all the comments were addressed. Let me know if there is anything else.

@gkdn
Copy link
Contributor

gkdn commented May 9, 2024

re. tests: It s kind of difficult to check all the coverage but do you mind double checking we have have coverage for

  • Having TrivialOnceFunctionCollector is important
  • we can handle multiple depth of call in single run
@@ -1,8 +1,8 @@
;; NOTE: Assertions have been generated by update_lit_checks.py --all-items and should not be edited.

;; NOTE: In real world example no-inline would use _<once>_ but there is escaping problem in a multi-platform
Copy link
Contributor

Choose a reason for hiding this comment

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

If escaping is fixed, this comment is now stale.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated j2cl-inline.wast to show that without the inlining it has the same result as before.

@gkdn
Copy link
Contributor

gkdn commented May 9, 2024

LGTM minus the comments. (one of them ended up being hidden behind resolved comments)

@rluble
Copy link
Contributor Author

rluble commented May 9, 2024

re. tests: It s kind of difficult to check all the coverage but do you mind double checking we have have coverage for

  • Having TrivialOnceFunctionCollector is important
  • we can handle multiple depth of call in single run

Both of those are already reflected in j2cl.wat.

The module at line 156 shows a transitive inline and hoisting that is 3 deep.
The module at line 209 shows inlining of once functions that do not have constants to hoist anymore, i.e. the effect of collecting.
The module at line 318 shows a more complex pattern (similar that what we generate) where constants are hoisted, the bodies are cleanup, collected and transitively inlined.

@rluble rluble requested a review from gkdn May 9, 2024 16:41
@gkdn
Copy link
Contributor

gkdn commented May 9, 2024

Sorry post an LGTM earlier but didn't notice I can mark it as approved. Thanks.

@kripken kripken merged commit a816627 into WebAssembly:main May 9, 2024
@rluble rluble deleted the j2clpass branch May 9, 2024 22:31
@gkdn gkdn mentioned this pull request Aug 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants