- User Since
- Nov 12 2014, 1:58 PM (191 w, 6 d)
Probably last round of comments. Thanks for your patience!
LGTM modulo minor.
FWIW, my take is that we should kill ResolvedUndefsIn (as already pointed by Eli, and probably me, and others in the past).
My take is that the benefit of having a forth lattice state is really little compared to the amount of effort we had to put to fix bugs.
If somebody is willing to take the time to do the work I'd rather zap away undef from the lattice and see how many constant we lose (and what's the runtime cost). But I understand this is a fair amount of work. That said, I think I like Eli's proof of concept a little better than this, so I'm happy if you want to pursue that path, Florian.
Fri, Jul 13
This is getting really close. Please try the lldbInline test format and revert the unrelated bits and I'll take another look.
Thu, Jul 12
Some comments. Jonas looked at many formatters recently so he's in a good position to take a look.
Wed, Jul 11
Ack'ed by Jim offline.
I have no more comments.
Tue, Jul 10
Was implemented differently. Closing.
Thank you Pavel!
Stefan (in our group) is going to look at CMake'y stuffs so I'm going to cc: him (he doesn't have a Phabricator account yet, so I just forwarded this e-mail to him for now).
Thanks for keeping us in the loop.
@labath this is breaking several bots for us internally so I would really love to get this in.
As this seems to be reasonably close to what you ask, I might consider checking this in before end of day (PST) today and we can iterate from there.
Mon, Jul 9
This was reverted at:
Davides-Mac-Pro:lldb davide$ git llvm push Pushing 1 commit: 116b1ec8e32 Rollback [test-suite] Add a decorator for the lack of libstdcxx on the system. Sending lldb/trunk/packages/Python/lldbsuite/test/decorators.py Sending lldb/trunk/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libstdcpp/iterator/TestDataFormatterStdIterator.py Sending lldb/trunk/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libstdcpp/list/TestDataFormatterStdList.py Sending lldb/trunk/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libstdcpp/map/TestDataFormatterStdMap.py Sending lldb/trunk/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libstdcpp/smart_ptr/TestDataFormatterStdSmartPtr.py Sending lldb/trunk/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libstdcpp/string/TestDataFormatterStdString.py Sending lldb/trunk/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libstdcpp/tuple/TestDataFormatterStdTuple.py Sending lldb/trunk/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libstdcpp/unique_ptr/TestDataFormatterStdUniquePtr.py Sending lldb/trunk/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libstdcpp/vbool/TestDataFormatterStdVBool.py Sending lldb/trunk/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libstdcpp/vector/TestDataFormatterStdVector.py Transmitting file data ..........done Committing transaction... Committed revision 336608. Committed 116b1ec8e32 to svn.
Fri, Jul 6
I went ahead and committed this because it was breaking a downstream bot:
Thu, Jul 5
The lldb bot started failing very recently and the blamelist hints at this change.
Mon, Jul 2
Fri, Jun 29
FWIW, I tried this on a lot of software in several context without having issues (including a kernel, the PS4 one).
If the kernel doesn't boot, I consider this a bug in the kernel.
Now, I really understand that this might be unfixable easily in the project, but I agree with Peter it shouldn't be the default.
Thu, Jun 28
Can you write a unittest for this? Thanks.
Wed, Jun 27
Fri, Jun 22
I think it's fair and correct supporting everything we can, but I guess llvm-gcc is largely dead at this point.
Thu, Jun 21
Wed, Jun 20
LGTM. @kuhar ?
Jun 17 2018
I admit I haven't looked at this code in a while, but I can't understand your fix. Can you please elaborate with an example?
Jun 14 2018
sigh at iterator invalidation issues. lgtm
Jun 12 2018
I didn't check whether the representation was correct (although it looks lo). The structure of the patch looks fine to me, thanks for adding the comments :)
Looks fine to me. Do you plan to use It in a subsequent patch?
I'll let @junbuml or @fhahn to make the call as they touched this more than me but I think we should see whether it's feasible to fix the algorithmic complexity (or understand why it's harder) before pushing this bandaid.
Unfortunate, but this seems reasonable. lg.
LGTM with a nit.
Jun 8 2018
I like the idea. Some comments inline.
It seems like this is trying to hide some algorithmic problem in the pass.
I think we should try to fix the pass instead. @trentxintong can you provide such cases?
Jun 6 2018
It seems reasonable to me.
May 29 2018
May 22 2018
Looks fine to me, but let's wait for Pavel. Are you back working on lldb ? :)
May 21 2018
Sorry Florian, I completely missed this one. LGTM as well.
Florian, I won't be opposed if you decide to preserve the dominator tree as part of changeToUnreachable, but, as we discussed with Dan in the past, do we really need to? I mean, we could just change changeToUnreachable (no pun intended) to do something sane and not modify the CFG (if the block is really unreachable, it's very likely that the next round of DCE is going to remove it anyway) [and in case it doesn't I'm really not worried about running DCE again because it's really basically free].
I skimmed quickly through these tests and they seem fine to me (at least, they document the current state).
I'd like to give @reames or @anna an opportunity to comment as they've working in this area more than I did lately.
If I recall correctly the sparse constant propagation implementation in LLVM (interproceturally, at least), calls changeToUnreachable which modifies the CFG.
(this is, quite unfortunate, as it's declared in Local.h). So, I'm afraid the patch as is is not correct. That said, it's been a while since I last worked on this area so I might recall incorrectly :)
May 18 2018
Can you commit the whitespace fixes separately?
May 17 2018
happy to see this going in when Adam is.
May 15 2018
This needs a testcase.
May 11 2018
The fix itself looks fine, but I'll defer to @aprantl.
Do you need anybody to commit this for you?
May 10 2018
This looks correct, but I'll let Kuba comment as he wrote this unittest originally.
May 9 2018
May 8 2018
Addressed comments. Thanks!
Simple enough. Gerolf?
Thanks for taking the time to split this!
Reduced again: https://reviews.llvm.org/P8080
May 7 2018
I think I found a better (but slow :() way to reduce it. I'll send an updated testcase tomorrow.
Got it down to 350 lines ~ with your suggestion, but still I'm not happy if I look at the size
May 4 2018
@mzolotukhin can you please take a look? This broke a lot in the past and you're the one who fixed it, so you're in the best position to review the patch.
May 3 2018
Does this fire anywhere? If so, we should try adding a test.