[Strings] Automate string lifting and lowering in the optimization pipeline #7540
Conversation
There was a problem hiding this comment.
There is currently a weird discontinuity in behavior here where input that includes a bunch of stringref operations will be unaffected by these passes, but then if even a single string import is added, all of the stringref operations will start being lowered away.
Can we solve this by having the lifting pass set liftedStrings if it encounters any stringref operations (i.e. "pre-lifted" operations) as well?
Can we also make it so the paired passes are the only versions? I don't think it's useful to provide both the paired and unpaired versions to users. We can have the PassRunner conservatively default to having liftedStrings=true so explicitly running only lowering still works as expected.
I agree that is a weird discontinuity, yeah. OTOH, another corner case is if the input has stringref but not imported strings. This PR leaves the stringref content alone in that case, which I think is natural (seems weird if you input stringrefs, optimize, and lose stringrefs), so changing things (to detect stringref regardless of imported strings) would regress that situation. Overall I think those corner cases matter less than the goal of avoiding extra work in the common case. This PR atm adds practically no work for
I think the user-facing passes are useful for people that use stringref directly. Which, maybe is almost or actually zero atm, but I see little harm to keeping them around, and as you know, I do believe stringref makes sense for wasm in the long term... 😄 (The paired version is for internal use, I can't see users wanting those.) |
|
After offline discussion, I added a nicer flag to disable the lowering |
| if (options.optimizeLevel >= 2 && wasm->features.hasStrings()) { | ||
| // Gather strings to globals right before reorder-globals, which will then | ||
| // sort them properly. | ||
| addIfNoDWARFIssues("string-gathering"); |
There was a problem hiding this comment.
Do we need string-gathering at all any more, or can we remove its code?
There was a problem hiding this comment.
We don't need it for users, though it is nice for testing. I can make it a testing pass as a followup?
After we lift the strings section into stringref, we don't need the section any more, and leaving it around could cause problems with repeated lifting/ lowering operations (which would be very much possible with #7540). In particular, without erasing it, we'd accumulate such sections over time.
-O2and above now automatically lift strings at the start, allowingoptimization in the middle, and lower at the end. This lets people
use imported JS strings and have string optimizations "just work",
if they enable the strings feature.
The best way I could figure out how to do this is to make a "paired"
set of lifting and lowering passes. The pair work together, in that
the lifting notes if it found anything, and if not, then the lowering
is skipped. If we do not do this then the lowering always runs, which
adds work (scan the entire module for strings), and equally bad it
also always rewrites types (to replace string with extern; avoiding
that work would require intrusive changes to TypeUpdating, or
even more work in some pre-scan).
TODO: document in optimizer cookbook if we agree on this path