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.

This is getting really close. Please try the lldbInline test format and revert the unrelated bits and I'll take another look.
Thanks!

Some comments. Jonas looked at many formatters recently so he's in a good position to take a look.

Ack'ed by Jim offline.

I have no more comments.

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.

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.

In D49033#1155743, @labath wrote:

Besides the problems I mention in inline comments, the other issue I have with this patch is that it does things completely different than how we implement the check for libc++, which is an identical problem. (libc++ detection is done by marking all libc++ tests as with a special category, and then deciding in dotest.py (checkLibcxxSupport) whether to run the category as a whole).

So, I think this should be reverted and replaced by a solution mirroring the libc++ approach. I'm not saying that one is perfect, but it is in many ways better than this one (e.g., if the default skipping logic gets things wrong, you can override it by manually skipping the corresponding category), and most importantly, it makes things consistent.

In D49033#1154767, @jasonmolenda wrote:

Is this going to work with non-clang compilers? ('$CXX -x c++ -stdlib=libstdc++ ') I can imagine gcc supporting these same flags, but will this work on windows?

I went ahead and committed this because it was breaking a downstream bot:

The lldb bot started failing very recently and the blamelist hints at this change.

In D48756#1149470, @tejohnson wrote:

This version is ok with me, question below around whether we want 2 separate options here though.

Davide, did you consider changing the default for the clang driver options instead of just in gold/lld for LTO? It might also be nice to hook up -fno-function-sections and -fno-data-sections to this new gold option (and mirror it in lld), just as I had done for the affirmative versions of those options in tools::AddGoldPlugin (which is now essentially a no-op due to the default change). In which case we would want 2 separate options here (one for functions and one for data). Thoughts?

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.

Can you write a unittest for this? Thanks.

LGTM.

I think it's fair and correct supporting everything we can, but I guess llvm-gcc is largely dead at this point.

LGTM.

LGTM. @kuhar ?

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?

sigh at iterator invalidation issues. lgtm

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.

In D47874#1126902, @fhahn wrote:

Or maybe it would be even more convenient to just have a new iterator type, which skips such meta instructions.

I like the idea. Some comments inline.

In D47874#1125517, @fhahn wrote:

In D47874#1125474, @efriedma wrote:

Not sure this is the best fix. The function is named findInsertPointAfter; from the name, I would expect it to behave similarly to BasicBlock::getFirstInsertionPt (which does not try to move past debug info).

We added instructionsWithoutDebug recently to deal with this sort of issue, but it's not directly usable here.

Yeah, maybe we should have a function that takes an iterator and advances the iterator to the first non debug/meta instruction?

In D47949#1126627, @fhahn wrote:

The pass currently walks back the single predecessors of a call site and records conditions relevant to the call site. There are cases where doing so we end up visiting the same blocks over and over again. It would probably be better to do a single traversal of the CFG and record relevant conditions once. I have to think a bit more about it, but maybe PredicateInfo could be helpful here and allow us to avoid visiting predecessors.

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?

It seems reasonable to me.

Looks fine to me, but let's wait for Pavel. Are you back working on lldb ? :)

In D47129#1108674, @kromanova wrote:

Good catch, thank you Craig! Did you know that you can reduce LLVM IR better than Delta? :)
Delta reduced the testcase quite well, but failed to remove this function.

Sorry Florian, I completely missed this one. LGTM as well.

In D47149#1106684, @fhahn wrote:

Right, IPSCCP does use changeToUnreachable, but it can be told to update the DT. The thing that confused me is that we use removeAllNonTerminatorInstruction... in runSCCP with a comment that we should not change the CFG. Is there a reason we use changeToUnreachable in IPSCCP but removeAllNonTerminatorInstruction... in SCCP?

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 :)

Can you commit the whitespace fixes separately?

happy to see this going in when Adam is.

@HsiangKai ping

LGTM

lg

This needs a testcase.

The fix itself looks fine, but I'll defer to @aprantl.
Do you need anybody to commit this for you?

This looks correct, but I'll let Kuba comment as he wrote this unittest originally.

Addressed comments. Thanks!

Simple enough. Gerolf?

Thanks for taking the time to split this!

Reduced again: https://reviews.llvm.org/P8080

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
https://reviews.llvm.org/P8079

In D46568#1090802, @aprantl wrote:

That testcase is huge. Do all instructions need !dbg locations?

@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.

lg

Does this fire anywhere? If so, we should try adding a test.