[NFC] Refactor Heap2Local logic #6473
Conversation
src/passes/Heap2Local.cpp
Outdated
| // Analyze an allocation to see if it escapes or not. We receive a Rewriter | ||
| // instance on which we store important information as we go, which will be | ||
| // necessary if we optimize later. | ||
| bool escapes(StructNew* allocation, Rewriter& rewriter) { |
There was a problem hiding this comment.
It's odd that the analysis depends on the rewriter. Would it be much work to factor out the parts of the rewriter that the analysis uses as well?
There was a problem hiding this comment.
It's more a matter of efficiency. To do this in a single pass versus two passes, it makes sense to check for escaping while we note what we need to fix up if we do find nothing escapes.
There was a problem hiding this comment.
Right, I don't mean that we should do this in more passes, but it would be nice to move rewriter.sets and rewriter.reaches out of the rewriter and into the analyzer instead.
There was a problem hiding this comment.
Hmm, interesting. Yeah, maybe a larger refactoring is worthwhile here. I'll look into that and update this PR with my findings.
|
Ok, I did a larger refactoring here that I think improves things significantly. There is now a core EscapeAnalysis class that does the actual analysis, and a separate optimizer that optimizer, and a separate parent that manages the relationship between them. Most code did not change or only changed slightly as fields were moved around. Unfortunately the reordering of code causes the diff to be large. This remains NFC. Also make a few things |
|
Btw, my goal in this refactoring is to make this pass also optimize arrays and not just structs. |
| // Maps indexes in the struct to the local index that will replace them. | ||
| std::vector<Index> localIndexes; | ||
|
|
||
| // In rare cases we may need to refinalize, see below. | ||
| bool refinalize = false; |
There was a problem hiding this comment.
Can we move these to be above the constructor so they are defined before they are used?
There was a problem hiding this comment.
Actually I intentionally put them below. We often have that pattern that arriving stuff is on top and internals appear later, like here. In particular this way shows a nice separation between the two groups. I suppose a comment could also achieve the same goal, but again, I think it might be less consistent with other code?
There was a problem hiding this comment.
I don't think I follow that pattern in the code I write 😅. Do we still follow that pattern when the internal data is used from the constructor? I do think it's generally nicer to have things defined before they are used, but I don't feel strongly about it here.
There was a problem hiding this comment.
I can't say I've been thinking about whether the fields are used from the constructor or not... 😄 Maybe it is more readable that way, I'm not sure. I also don't feel very strongly here.
I already have a followup PR here, so maybe let me land this and we can consider the ordering here in that PR? This question will appear a second time there (I am adding another class much like this one).
Separate the escape analysis portion from the part that does the optimization.
We do still record information during the analysis phase, but at least this makes
the separation clearer, and will also allow analysis without necessarily optimizing.