- User Since
- Nov 12 2014, 1:58 PM (174 w, 3 d)
Thu, Mar 15
So excited to see this happening! :)
Wed, Mar 14
(please wait a day or two if @labath has comments) but this should be fine.
lgtm, we do the same exact thing in ELF, I don't see why COFF should be different.
Also, I second the feeling of having lldb somewhere in the name for the utility (rather than llvm :)
The name should be changed (also the utility name), but that should be done separately.
Tue, Mar 13
Mon, Mar 12
This is No functional change, right (just code churn)? If so, LGTM.
Sat, Mar 10
Fri, Mar 9
This patch has no testcase. It shouldn't be particularly hard to write one, you can take inspiration from the one in lit/.
Thu, Mar 8
I'm going to review this in detail, but I have a general comment about the design.
I'm a little torn about the approach you're using. I think it's a good experiment because you're leveraging the structure of the problem to find a fundamentally more efficient solution. This is, FWIW, not really surprising.
OTOH, this is bad because you're de-facto rewriting a specialized SSA updater. We might need another one for, e.g. LCSSA, and another one for GVN/PRE (until NewGVN is in or we rewrite PRE to not use the updater).
This results in basically a bunch of code duplicated. History shows (and you know this very well) that it turned out to be a long tail of bugs when we did something similar with the dominator (have specialized updates per-pass). If we really want to go this route, we really need to analyze carefully all the implications.
std::shuffle has linear time complexity, so I'd rather not pessimize the DEBUG build even more.
As discussed, given you're moving the whole file anyway, this is fine. Thanks
Wed, Mar 7
Marking as requested changes to get off of my queue. You can commit the formatting changes without review, then update this patch (I'll take another look).
Makes sense to me, can you do the formatting separately?
Tue, Mar 6
Mon, Mar 5
Fair, I don't have a strong opinion on whether this should be in an header or not. Probably Greg is right though, if this is not used anywhere else, we could make it somehow private.
Sun, Mar 4
Sat, Mar 3
LGTM, thanks Rafael.
This seems scary. We really need a test case for this.
LGTM with Pavel's suggestion implemented.
This looks correct to me but please wait for Clement's opinion on it.
Thu, Mar 1
As already pointed out, I think this feature should be thought again & have more focused testing. We can have a meeting/discussion about this, and I need to think about it more.
But what you're proposing is basically a sledgehammer and I don't feel confident about having the patch committed in its current state.
Wed, Feb 28
LG modulo the test. Update that, I'll take another look and approve it. Thanks for your contribution!
I like the way this patch is structured, some comments inline.
LGTM too. Sorry I'm a little behind with reviews because life took over.
Tue, Feb 27
LGTM modulo comment. Adrian?
Mon, Feb 26
This commit has no tests. It should have many. It's very big, so it could be split in several pieces.
I'd really appreciate if you can take the time to do so. For now, some comments.
Just noticed you added it later, never mind.
Can we add a test for this?
Sun, Feb 25
fwiw, you don't need unit tests or python tests to implement this.
If I understand the feature correctly you can probably extend lldb-test (which is run as part of check-lldb-lit).
Up to you though.
Fri, Feb 23
(and thanks for saving 1 minutes and 30 seconds of my life multiplied by the many times I run the test suite per day).
I was going to suggest the same thing Zach suggested, but I think this fine as is.
LGTM. The fact the test is more concise is definitely a win, but I don't think this is the main reason for doing the conversion.
The 300x speedup makes this change a no-brainer.
Thu, Feb 22
LGTM, I didn't think about modulo when I did this for integer division, but, as you say, it's a natural extension.
Wed, Feb 21
I wonder whether we could use something like
>>> import os >>> os.path.basename('/patatino/ino/main.c') 'main.c'
Tue, Feb 20
The meta comment I have here is whether we need to segregate this is in dsymutil or expose that somewhere else (e.g. in Support/).
My feeling is that this utility could be of more general use for pathname resolution.
Among others, lldb calls realpath a bit, and maybe there are components in clang doing the same. Jonas, what do you think?
LGTM. This is really good work, thanks
Mon, Feb 19
yes, please use Range loops here. Otherwise the idea is fine.
Jonas, this looks a good use case for using lit.
Is it possible to reuse the machinery we use in lldb/lit/Expr ?
If not, well, we know there's something we can improve :)
lg, thanks for taking care of this.
LGTM, thanks for doing this :)
Sat, Feb 17
Fri, Feb 16
Some high level comments:
Feb 14 2018
After all the work he did on the testsuite I think Adrian is in a good position to review this one.
Feb 13 2018
+ @mzolotukhin We have a test where LCSSA takes forever, we can measure the impact on that one, Michael is looking at this currently.
This is fine, but haven't looked whether DSE has other places where it removes instructions.
Feb 12 2018
I think this Is in line with what Gerolf had in mind, but I'll let him sign off this one.
Feb 9 2018
LGTM if Adrian/Vedant are happy with this.
marking as "requesting changes" so it gets out of my queue
Did you get a sense of why we can't remove the instructions? This is probably something we want anyway, but there could be another bug in llvm (or bugpoint itself, as instruction deletion is always supposed to succeed).
LGTM modulo minor.
Feb 7 2018
Can you add a unittest for this? :)
You can take a look at the test/testcases/functionalities/completion/TestCompletion.py for the python equivalent. I find the potential FileCheck'ed version much easier to read/write/understand.
I'm possibly biased having worked many years on LLVM, hence I'm asking for general feedback.
thanks for fixing this.
Feb 6 2018
LGTM. One minor comment inline. Feel free to decide whether you want to address & commit (no need for another round trip)