ProfileSummaryInfo is updated seldom, as result of very specific
triggers. This patch clearly demarcates state updates from read-only uses.
This, arguably, improves readability and maintainability.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Could you separate all the constification (& possibly/as much of the renaming as possible) from the semantic changes here?
(no need to send the constification for pre-commit review - feel free to commit it before or after the semantic changes depending on what suits/is practical/etc)
llvm/include/llvm/Analysis/ProfileSummaryInfo.h | ||
---|---|---|
66 | I'd probably write this as Summary != nullptr? |
I'm not sure why.
+1
llvm/include/llvm/Analysis/ProfileSummaryInfo.h | ||
---|---|---|
44 | +1 | |
62 | samples -> summary? | |
63 | refresh() won't read again once once a non-null summary is created. Do we anticipate a situation where the profile summary in the IR may be set once and then updated later perhaps conditionally? Who should call refresh? | |
llvm/lib/Transforms/IPO/SampleProfile.cpp | ||
1851 | This if statement may suggest that we may not always set it here or may have been set at an earlier point (do we know if that's possible)? Should we move the refresh call inside the if statement? |
feedback
llvm/include/llvm/Analysis/ProfileSummaryInfo.h | ||
---|---|---|
63 | That's currently the case, too - see the old computeSummary() def and uses. Re. the second question (if we anticipate a situation...) - my goal with this patch is to help make such analysis simpler, by clarifying where the transitions happen. When such a scenario appears, the agent invalidating the profile should trigger the refresh. I think, ideally, we'd control the state of the PSI from the pass pipeline through explicit pass calls - it'd make it even more clear what the state is at any given moment. This patch is a step in that direction. | |
llvm/lib/Analysis/ProfileSummaryInfo.cpp | ||
97 | We still need to go and compute the thresholds. | |
llvm/lib/Transforms/IPO/SampleProfile.cpp | ||
1851 | That's more clear, indeed. |
I still think it'd be best to have the pure formatting changes (clang-format) in a separate patch.
llvm/include/llvm/Analysis/ProfileSummaryInfo.h | ||
---|---|---|
63 | I think there are two approaches here:
Which are we going with? If 1, would it be useful to make calling refresh again after it's filled into an assert error? Currently it gets dropped silently, but it would catch if someone updates the PS in the IR and tries to update the PSI, but not really updating it. If 2, would it be useful to make refresh update PSI every time it gets called? (which we could do for 1 as well?) |
I think it's a matter of what scenarios we have: do we have a scenario for ever re-updating the PSI or not.
We do need to work that out, and that's my intention. In the meantime, I believe it would be helpful to simplify the current state of the code. As this patch clarifies state transitions, it should be relatively easy to further simplify it further afterwards.
may be better to keep this helper for readability.