- User Since
- Jul 8 2015, 10:26 AM (184 w, 3 d)
Thu, Jan 17
Gentle ping (this isn't urgent, but it seems nice to have).
Wed, Jan 16
(Looks reasonable to me. I haven't really touched this code, so it'd help if another review could +1 the change.)
Fri, Jan 11
Thanks for the patch.
Thu, Jan 10
Mon, Jan 7
Looks like a nice/reasonable cleanup, thanks!
+ @gyiu, as I believe he's worked on the partial inliner.
Thu, Jan 3
Mon, Dec 31
Thanks for working on this.
Fri, Dec 21
- Tighten up the tests a bit.
Thanks, this patch lgtm.
Thanks for the patch!
- Address feedback from @davidxl
(Marking this WIP while I address @davidxl 's feedback.)
Dec 20 2018
Dec 18 2018
- Simplify logic per Eli's review feedback.
Dec 17 2018
I think it'll be really useful to have this helper around. Thanks, lgtm with the name change!
Dec 15 2018
@aprantl It looks like the percentage of variables with location went up right after this patch landed:
Thanks for working on this :). It’d be interesting to see some pre/post patch compile time numbers on CTMark.
Dec 14 2018
Dec 13 2018
I think this patch is complete, it just needs review.
Apologies, I haven't been able to get back to this. I'd be happy to have someone commandeer this.
Dec 11 2018
+ 1 to this. If there's a tidy plugin for misleading indention, that might address some of Adrian's concerns.
Dec 7 2018
Looks great, thanks.
LGTM. @CarlosAlbertoEnciso, any thoughts on this?
LGTM, thanks! I'd suggest running through debuginfo-tests if you haven't already (ninja check-debuginfo).
Dec 6 2018
Thanks for the patch, this is a great catch.
Dec 3 2018
Friendly ping. I've rebased this on top of r348205, which fixes the assertion failure pointed out in llvm.org/PR39564.
Thank you, LGTM.
Nov 30 2018
Nov 29 2018
While this patch is correct, it might make sense to abandon it in favor of https://reviews.llvm.org/D55018, which solves the underlying issue more comprehensively.
Nov 28 2018
Thank you (very much!) for working on this.
Nov 27 2018
- Use an iterative approach to remove blocks from the outlining region, if they would cause PHIs outside of the region to become invalid (i.e. have more than one distinct incoming value from the codeRepl block).
Nov 26 2018
@jessicah Please ping this review if you need someone to commit this patch on your behalf.
Nov 20 2018
For compiling/linking, I think we can get by using lit substitutions to fill in platform-specific options? iOS testing for Swift is done this way (both on-device and simulator), as is testing for the profiling runtime. Dan and @filcab are more active in the area of sanitizer runtime testing, so they may have more informed opinions to share about how well that model works.
Nov 18 2018
Imho CodeExtractor needs a little more work before it's ready to be on-by-default. There are two main blockers: missing debug info support, and a missing whitelist of extract-able intrinsics. The latter poses a high risk for miscompiles (see llvm.org/PR39545, llvm.org/PR39671).
Friendly ping -- @orivej were you still looking for more feedback? If not, do you still need someone to land this patch on your behalf?
The history seems complicated. I think it'd be really useful to sort out why getParenOrBraceRange() couldn't give the right result, but I'd be happy to see this land first to address the crash.
I agree that this looks a bit more consistent with the rest of the codebase. LGTM, thanks.
Nov 16 2018
Nov 15 2018
Could you share either a link to or the abbreviated text of the test failure?
Nov 14 2018
Actually, when I defined “IsCold” as “has more than one predecessor” to stress-test the pass, I found another instance of this assert firing while building a stage2 clang.
Ping, are there any outstanding concerns about this one? It'd be nice to have this in-tree, as I have a few follow-ups based on it.
- Handle cold invokes, and address review feedback from @junbuml
Nov 13 2018
Nov 10 2018
Nov 9 2018
- Split out the change to mark llvm.trap cold, per review feedback.
Nov 8 2018
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.
Nov 7 2018
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.