[NFC] Move optimizeSubsequentStructSet() to a new pass, HeapStoreOptimization #6882
Conversation
src/passes/pass.cpp
Outdated
| @@ -209,6 +209,8 @@ void PassRegistry::registerPasses() { | |||
| createTypeRefiningPass); | |||
| registerPass( | |||
| "heap2local", "replace GC allocations with locals", createHeap2LocalPass); | |||
| registerPass( | |||
| "hso", "optimize heap (GC) stores", createHeapStoreOptimizationPass); | |||
There was a problem hiding this comment.
Can we give this a more descriptive flag name? If you want to keep the short name, perhaps we can make it the single dash name. (I don't love our current system of single dash names since it violates linux command line conventions, but that's ok.)
There was a problem hiding this comment.
We don't have a mechanism for giving a pass both a long and a short name - all passes have just one way to invoke them.
In this case --heap-store-optimization seems quite long so I thought --hso seemed better, but I don't feel strongly if you prefer the verbose way?
There was a problem hiding this comment.
(and --hso is in the spirit of --dce etc.)
There was a problem hiding this comment.
Whoah, I wonder why I thought we had separate short and long names. Did we ever?
Personally, I would prefer --optimize-heap-stores. Things like DCE and LICM are established acronyms that anyone familiar with compilers would recognize, but I think we should avoid making up new ones.
There was a problem hiding this comment.
We do have short names for tool flags, maybe you're thinking of that? And pass flags are added to tool flags. But pass flags never had a short version.
but I think we should avoid making up new ones
I'm curious why? Yes, --hso is definitely unfamiliar to a newcomer, but --heap-store-optimization is not that much better (optimize how? heap in what sense?). I think we quickly get used to short names and it saves typing, e.g. has --dae ever confused you?
And I do think typing less is nice here. We only write the full name in the pass once when writing it, but may write --foo-bar to run the pass 1000 times later (during testing, investigating fuzz bugs, etc. etc.).
With all that said, I'm ok changing it to a longer name. But I'd prefer --heap-store-optimization over --optimize-heap-stores. The former is consistent with classical optimization names, like "dead code elimination".
There was a problem hiding this comment.
We do have short names for tool flags, maybe you're thinking of that?
Ah yes, I must have thought this applied to passes as well.
And I do think typing less is nice here. We only write the full name in the pass once when writing it, but may write --foo-bar to run the pass 1000 times later (during testing, investigating fuzz bugs, etc. etc.).
That's fair. A nice solution might be to register the same pass under both a long name and a short name and document the short name as just being an alias for the long name in the help description.
But I'd prefer
--heap-store-optimizationover--optimize-heap-stores.
That's fine with me.
There was a problem hiding this comment.
but I think we should avoid making up new ones
I'm curious why? Yes,
--hsois definitely unfamiliar to a newcomer, but--heap-store-optimizationis not that much better (optimize how? heap in what sense?).
The long name is at least more descriptive than the short name. Even if it doesn't convey all the details, it at least conveys what part of the code is being optimized.
I think we quickly get used to short names and it saves typing, e.g. has
--daeever confused you?
I looked through the names of all the passes, and without looking at the descriptions I did not know or remember what --dfo, --nm, or --rse did, nor is there enough information in those names that I could have made an educated guess. Even for those I do know, like --dae, I always have to pause for a moment and recall what they stand for.
There was a problem hiding this comment.
Ok, renamed to --heap-store-optimization.
| auto* localSet = list[i]->dynCast<LocalSet>(); | ||
| if (!localSet) { | ||
| continue; | ||
| } | ||
| auto* new_ = localSet->value->dynCast<StructNew>(); | ||
| if (!new_) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
It would be nice if we could extend our match.h infrastructure to handle cases like this (but no action necessary of course).
This does not use the CFG yet, so there is no benefit (and likely some small slowdown). The next PR will actually use it to fix a correctness bug. This PR only sets up the CFG and converts the pass to operate on it, without changing any behavior or tests. Followup to #6882
This just moves code out of OptimizeInstructions to the new pass. The existing
test is renamed and now runs the new pass instead. The new pass is run right
after each
--optimize-instructionsinvocation, so it should not cause anynoticeable effects whatsoever, making this NFC.
The motivation here is that there is a bug in the pass, see the new testcase
added at the end, which shows the bug. It is not practical to fix that bug in
OptimizeInstructions since we need more than peephole optimizations to do
so. This PR moves the code to a new pass so we can fix it there properly,
later.
The new pass is named HeapStoreOptimization since the same infrastructure
we will need to fix the bug will also help dead store elimination and related
things.