- User Since
- Jul 21 2014, 12:07 PM (160 w, 6 d)
Fri, Aug 18
LGTM. Are you going to commit this as is or fix the new PM -pass-remarks-output first and update this? I am fine either way.
Mon, Aug 14
LGTM. Please watch out for any failures.
Fri, Aug 11
LGTM, with the same note here: as long as it still works with Python2 (which I am less sure of in this case).
LGTM, assuming it still works with Python2. (I don't see why it wouldn't.)
LGTM, sorry :(.
Thu, Aug 10
Wed, Aug 9
Tue, Aug 8
Mon, Aug 7
Thanks very much for working on this! Please assign https://bugs.llvm.org/show_bug.cgi?id=33794 to you. Also check out my note in the bug.
Sun, Jul 30
Sat, Jul 29
LGTM, I suppose there is already a -pass-remark test for this. Of course it would be great to also have a YAML test.
Thu, Jul 27
I only have high-level comments at this point because I don't think we're quite on the same page yet.
Wed, Jul 26
Tue, Jul 25
Really minor nit below. No need to upload the patch after this change, just go ahead and land it.
LGTM with a minor request. Thanks!
Jul 19 2017
Also with https://reviews.llvm.org/rL308537, opt-stats now prints a metric for memory consumption if you have guppy (a.k.a. heapy) installed. The metric is the total memory allocated divided by the number of remarks -- average in-memory remark size (~3KB currently). This is printed once all the remarks are loaded.
I am not really familiar with MIR test files but this looks straight-forward. LGTM.
Looks good, how hard would be to add a test for this?
LGTM once we clear the question below. Thanks!
Before writing out the difference YAML file in opt-diff, we need to recover
the YAML-friendly data structure for the remarks.
I have no further comment beyond Florian's and Davide's comments. Thanks for working on this!
Jul 18 2017
Yep, looks good.
Forgot David Li from the original list.
Jul 17 2017
@davide, this version seems to correctly exclude mounted dirs, what do you
Jul 13 2017
LGTM with one nit below. Thanks!
Jul 12 2017
Jul 7 2017
Thanks for adding this!
Jul 6 2017
Jun 30 2017
Looks great, thank you!
LGTM now, thanks!
Jun 29 2017
LGTM, one suggestion below.
This is great! I am wondering if you looked at actual progressbars. For example looks like that tqdm might not be hard to adapt to multiprocessing (https://stackoverflow.com/a/40133278). What do you think? I am happy to take this as is if you don't want to spend more time on this for now. This is already great "progress" :)
Jun 25 2017
Jun 23 2017
This may be way less efficient for Python 2. You may want to follow the guidance from here:
Jun 10 2017
Jun 5 2017
Jun 2 2017
Thanks very much for rewriting the loop. This is way more intuitive now.
Jun 1 2017
May 31 2017
May 30 2017
This version handles edge-dominance in the presence of non-unique edges.
May 26 2017
This is going in the direction, IMO. Would also be good to see the clang patch at some point. Thanks for tackling this!
I certainly agree that if we're not returning false for the non-unique edge case then that will cause bugs later on.
Removed the asserts. As Danny put it:
@davide, I have an ll file for you. On my box, with an assert-enabled opt it gives: