- User Since
- Apr 9 2013, 4:37 PM (381 w, 6 d)
Jul 11 2019
I'm currently testing this patch. Will be committing after all tests are done.
Jun 25 2019
Mar 5 2019
Oct 3 2018
May 11 2017
Apr 26 2017
Oh, so DAG vs DAG-NEXT, right?
Feb 14 2017
Dec 14 2016
Oct 7 2016
Thanks, Dehao. This LGTM now. I'm not 100% sure about the code paths in *Dwarf*.cpp, but they look safe enough. The only concern there would be any other code that also calls getDiscriminator, in which case, perhaps we could sink the DWARF version check into getDiscriminator and have getDiscriminator return ErrorOr.
Oct 3 2016
Sep 21 2016
Sep 19 2016
Sep 18 2016
OK, so instead of looking at the number of samples collected at the call site, it looks at the number of calls the target had. But what if the target is called disproportionately more from one call site than the other?
Apologies for the delay.
Aug 26 2016
Aug 12 2016
Aug 11 2016
Apologies for the delay, Dehao. Thanks for the description. That needs to be added to the code, so we have it for future reference. I suspect that several of the spots where I was confused and asked for comments are going to be good candidates to sprinkle that high-level description around.
Aug 8 2016
Could you describe this in *much* more detail?
Aug 5 2016
Adding Dehao. ISTR something special about 0 counts in profiles, but I don't recall details. Dehao, this looks fine to me, but will it break anything for samplepgo?
Jul 19 2016
Any changes in build time? I guess I'm surprised that removal operations are more frequent than lookups.
Jul 11 2016
LGTM with a couple of small changes.
Jun 27 2016
Jun 23 2016
LGTM. Thanks for the testcase and description.
Jun 22 2016
Jun 21 2016
Jun 15 2016
Thanks for the test case.
Jun 9 2016
A small test case, perhaps?
I agree with Igor's analysis. Let's first fix the correctness issue. Any compile-time problems that show up, can be fixed with a data structure change later. Igor, do you have any test cases that show the problem? Without a test case, it's harder to justify this change.
May 24 2016
Sounds like a great plan. Thanks for doing this!
Ah. I should'be read this one first :) Ignore my question on http://reviews.llvm.org/D17742, then.
Now, if the sample profiler depends on instcombine, we'd have to schedule it after instcombine, right?
May 5 2016
OK, sure. What I meant is a testcase that motivates this change.
Would it be possible to get a test case where we miss discriminator placement if simplifycfg doesn't run beforehand?
Apr 29 2016
I was going to ask about a test case, but we already have test cases that cover this, right?
Apr 25 2016
LGTM. Apologies for the delay.
Apr 20 2016
I have started going over this patch, but my knowledge of this area is pretty spotty. I defer to Duncan for a better opinion on this.
This looks fine, but let's wait until Easwaran's inliner support lands before submitting this change.
Apr 14 2016
Looks much simpler. Thanks!
Mar 28 2016
Nothing here seems to change the format of the external file, but could you double check with the AutoFDO converter that we can still read Perf profiles?
Mar 10 2016
Needs a test. Otherwise, LGTM. Thanks.
Mar 2 2016
Just to make sure AutoFDO is still working, would you mind building it with your LLVM build to make sure that create_llvm_prof still works?
Nice cleanup! I think we can prevent breaking the AutoFDO tool by adding an overload.
Mar 1 2016
Could you give me a bit more context? A C/C++ motivating example would be great. I think I follow what the intent is, but I'd like to make sure and leave it documented in the commit log.
Feb 29 2016
LGTM. Thanks for the fix!
Feb 26 2016
Looks fine to me, though I'm not an expert on MD.
Feb 22 2016
Feb 18 2016
Thanks for the new feature! LGTM with one minor nit.
Feb 17 2016
Feb 16 2016
I just started looking at this. Some early feedback so we can discuss whether my idea makes sense. Mostly, I'm thinking we want to support summary data in text profiles for testing and debugging. Does that make sense to you?
Dec 17 2015
Dec 16 2015
Dec 14 2015
Nice. Thanks. The shortened names are indeed easier to read (in addition to the size savings).
I like the new way of specifying weighted input better. Thanks! This LGTM with a couple of changes below.
Dec 11 2015
Dec 8 2015
Dec 7 2015
Once this is in, please update lib/Transforms/IPO/SampleProfile.cpp. It does something similar.
Dec 4 2015
I've just started looking at this, so these first few comments are mostly for my benefit to make sure I'm understanding this.
Nov 29 2015
Nov 27 2015
Nov 26 2015
How is this going to interact with SamplePGO? Currently, I'm testing a change that adds the InlineHint attribute for functions that globally collected a fraction of samples greater than a given threshold. Likewise, for functions that fall below another threshold, they receive the Cold attribute.
Nov 24 2015
Nov 23 2015
Nov 20 2015
Nov 19 2015