- User Since
- Jul 8 2015, 10:26 AM (174 w, 6 d)
Sat, Nov 10
Fri, Nov 9
- Split out the change to mark llvm.trap cold, per review feedback.
Thu, Nov 8
Why not check in a .o? The numbers won't change, so they can meaningfully be compared.
This looks good to me. I think it'd help to have a +1 from Adrian, as he's worked on this file much more than I have.
Oh, I just realized this also needs an entry in CommandGuide (I think the file is called llvm-cov.rst).
Thanks for the patch. Would you mind uploading a diff with additional context (e.g. git diff -U10000)? Phab doesn't render it.
Hi Rong, at a high-level, I like what this patch is doing. I'll try to leave in-depth comments by next week -- please ping the review otherwise.
I suggest adding a note in docs/ReleaseNotes.rst about this.
Wed, Nov 7
Looks reasonable to me, but it'd help to check in a .o for a test.
- Do not add duplicate blocks to the outlining region. The test for this is 'forward-dfs-reaches-marked-block.ll'.
- Address review feedback from Aditya.
LGTM, although I don't think the 'pr39187-gline-tables-only.ll' adds any additional test coverage, so I'd suggest deleting it. The other test already tests the interesting case (dbg intrinsic prior to terminator with a different location).
Tue, Nov 6
Removing the WIP label, as I'm confident in the results now.
Mon, Nov 5
Sun, Nov 4
Fri, Nov 2
- While doing performance testing I found a miscompile in ./SingleSource/Regression/C++/EH/Regression-C++-class_hierarchy. I'll file a PR with more details by next week. It looks like it could be an existing bug that surfaces due to more aggressive outlining.
- Added tests (including the one from @sebpop's earlier patch).
Wed, Oct 31
Hi Teresa, with this patch applied, I do still see a (possibly unrelated) crash when building an internal framework with ThinLTO + hot/cold splitting. I'm not sure what the best way is to prepare a reproducer (please let me know), but I've tried to collect some output from the debugger:
Tue, Oct 30
Marking the tests are 'unsupported' would be a good way to surface the warning, as the number (and possibly the list?) of unsupported tests is printed out at the end of each test run.
Mon, Oct 29
- Move a test that's particularly X86-specific into an X86 directory.
Eli points out that D53514 will improve the codegen for the use case I'm most interested in, but in a way that makes this patch a bit less relevant. While it should still be useful to deal with actual VLAs, I'll need some other solution to allow outlining calls to os_log.
Fri, Oct 26
- Fix the issues Eli pointed out.
Don't you just want to reset the counters in the child process?
Even if there's a file at the path we're supposed to remap, shouldn't we still do the remap? Couldn't the file we're intended to read be at the remapped path?
Thu, Oct 25
The verifier bits look great, I'll let someone else comment on the functionality changes. Thanks!
@sebpop are you interested in rebasing this on the new cold block propagation code? If not, I'd be happy to give it a try. Is the main challenge in teaching CodeExtractor to update the DT and PDT?
Wed, Oct 24
- Rebase to incorporate naming changes from https://reviews.llvm.org/D53534 into the tests.
- Disable a problematic CodeExtractor unit test. CodeExtractor miscompiles the code in the test. Any time there are live output values from an outlined region, there's a chance CodeExtractor will leave around a phi node with incoming values from a different function.
Fixed a bug in CodeExtractor found by building the test suite with an asserts-enabled compiler.
I've been fuzzing+testing this patch further and found a verification failure in CodeExtractor. As we now support outlining regions with multiple exits, CodeExtractor cannot leave around duplicate incoming values from the codeRepl block. I think this should be simple to fix.
Thanks for the review.
LGTM, thanks. I'm sorry for the delay in reviewing this.
Tue, Oct 23
- Before this patch, 47KB of text was outlined in a test-suite run (X86, -Os, LNT + externals). After this patch, 134KB was outlined, so roughly a ~3x improvement.
- Fix a typo in a comment.
Thank you! For completeness, it might help to have a test for the case 'Phase = PostLink'. Happy to leave it to you to decide.
Thanks for the patch! Your explanation makes sense to me, but I'm not familiar enough with this code to give a +1. @steven_wu, any thoughts on this?
@sebpop thanks for this patch! I don't see any problems with it (although I would prefer that the test explicitly check that outlined functions contain the correct instructions).
Mon, Oct 22
Thanks for your notes @tejohnson.
Sat, Oct 20
Why not use RAUW to fix up any metadata which refers to a function?
Looks great, thanks!
Fri, Oct 19
Could you add a test for this similar to 'test/Other/opt-Os-pipeline.ll'?
Mon, Oct 15
Hi @tejohnson @Sunil_Srivastava, just a heads-up that with https://reviews.llvm.org/rL344545 applied, I'm hitting "Invalid function metadata: outgoing forward refs" in an internal project with ThinLTO and hot/cold splitting enabled. There's some sort of problem caused by having an outlined function with debug locations in it but no DISubprogram. I'll try to dig a bit deeper.