User Details
- User Since
- Aug 7 2020, 3:53 PM (23 w, 4 d)
Fri, Jan 15
Thu, Jan 14
Add DEFAULT testing to make sure baseline inlining differs from replay. Fix copy-paste error in flag description for -cgscc-inline-replay
Tue, Jan 12
Mon, Jan 11
Add comment about best effort approach of replay.
The modifications to the DefaultInlineAdvice is an attempt to solve the following problem:
- In the SampleProfile inliner it emits remarks through the "legacy" interface with emitInlinedInto. For replay if advice generates remarks you'll get duplicate remarks but if it doesn't you get a single set (which is what's happening today and correct).
- In the CGSCC inliner in newPM it emits remarks through InlineAdvice via recordInlining and nowhere else. For replay if InlineAdvice generates remarks you're good but if it doesn't you get no remarks.
Fri, Jan 8
Move the ReplayInlineAdvisor.cpp/h and SampleProfile.cpp files to D94333 as they need to be atomic with the remarks format change.
Split changes with D94334 better: now the ReplayInlineAdvisor update which needs to be atomic with the formatting change is moved over to here. Removed ReplayInlineAdvice and modified DefaultInlineAdvice to provide the desired functionality. Fixed up tests.
Dec 3 2020
Appreciate the quick commit! Guess I gotta be faster with D92592 :D.
Good catch @MaskRay. If I'm understanding correctly I think the correct approach is to move the class AliasScopeNode I need to metadata.h to fix this?
Certainly, I appreciate the thorough and quick review! Best of luck with your restrict patches!
Dec 2 2020
Updating new merge test to explicitly look for the correct merged scopes. Thanks @jeroen.dobbelaere!
Dec 1 2020
Go with intersection of domains then union the scopes within those domains as discussed. Updating tests to match latest behavior.
Nov 30 2020
Move the fix to getMostGenericAliasScope. Renamed metadata in test case.
Nov 19 2020
@jeroen.dobbelaere I think the correct merging in all cases requires this strategy (or one which checks that all the domains match). If true (@hfinkel?) that'll be a larger change that needs more testing.
Changing to conservative merge of metadata rather than bailing on optimization. Updated test case due to that.
Nov 18 2020
Update unit test with correct set of metadata
Thanks for verifying! Good catch, I'll update the patch with this fixed up example.
I've looked more into what alias.scopes are and I think I've got a better handle on what's actually wrong. In the meantime let me try to describe the original problem better. The first section is checking my understanding and making sure we're on the same page. Skip to the second part for the bug details.
My understanding of domains
Let's say we have the following (I slightly modified test/Transforms/Inline/noalias.ll for this):
Nov 16 2020
Add more comments in test case
Oct 12 2020
@MaskRay Ah the action table start is inferred as the end of the CST. Saves a pointer but shackles the implementation. These tables will be fairly small (effectively 1 entry per "catch" in a function) and not all CST would need a corresponding action table (only those inside "try" do) so duplicating them is not as far-fetched as it would initially sound.
Oct 9 2020
Remove useless "if"
Oct 1 2020
Glad to see this landing! I'm testing locally with my change to enable this purely for landing blocks.
Sep 25 2020
Sep 17 2020
Sep 16 2020
@rahmanl Have you had a chance to run your added test on ARM64 as another itanium C++ ABI target to make sure it works properly? Having testing there is fine as a follow-up but wanted to make sure we're in agreement here. Otherwise changes LGTM, please let @MaskRay have a chance to provide any additional feedback.
Thanks @asbirlea!
Sep 15 2020
Rebase #2
Sep 14 2020
Remove unnecessary period
Fix merge issues with comment, updated description to include benefit we see from this change. Thanks @asbirlea for the quick review!
Sep 11 2020
Rebase
Sep 10 2020
Sep 9 2020
These changes will also apply to other Itanium C++ ABI targets (arm32/arm64/RISCV etc.) so adding testing for at least another target is good. Trying this out for arm64 hits an ICE here:
case TargetOpcode::EH_LABEL: if (MBB.isBeginSection() && MBB.isEHPad() && ((*std::prev(MI.getIterator())).getOpcode() == TargetOpcode::CFI_INSTRUCTION)) {
which is worth following up on.
On this front, is there a specific reviewer that we're waiting for here to get this change through?
Sep 3 2020
Looking at https://bugs.llvm.org/show_bug.cgi?id=36578:
Andrew Kelley 2019-08-11 09:00:59 PDT
I'm no longer subscribed to this bug report. I've come to the conclusion that LLVM's coroutine API is not worth using, and resorting to implementing coroutines directly in the frontend.
Sep 2 2020
I think with this alongside the recently committed MachineFunctionSplitter (MFS) the MFS phase is a good place to split out EH landing pads in both profile and non-profile code. A small change in MFS will enable EH LP splits in profiling given they should always be cold. I think in non-profile mode a sub-flag can be added like -mfs-split-LP-only where EH LPs are always split out. Enabling a general static-analysis splitting could also be interesting but outside of EH LPs I'm not certain there's guaranteed gains in other scenarios.
Sep 1 2020
I'm rebased against 478eb98cd25cb0ebc01fc2c3889ae94d3f1797d3 and I'm seeing both added tests failing. They may need to be updated.
Aug 28 2020
Remove redundant VH callback as @nikic helpfully pointed out!
Aug 27 2020
Condition usage of BFI to PGO in newPM as well
only use BFI when profile is enabled, have LICM mark BFI as preserved
Aug 26 2020
Remove usage need for BFI in LPM2 and set unswitching to preserve lazy BPI/BFI so it can remain in the same loop pass as LICM
Change to LazyBFI for legacy pass manager to prevent rebuilding the post-dominator tree
Aug 21 2020
@asbirlea Thanks for taking a look!
Aug 18 2020
Commit my changes (crazy I know) so that the diff is actually updated for linting
Linting