Rebased.

LGTM, but please add CHECK-NOT into test to make sure that the metadata is actually dropped.

Rebased.

Ping?

I would ask one more round of review for changes made to LangRef. I tried to make it more generic and less reliant on deopt's specifics than it used to be.

Made changes to LangRef to make the widening part independent on deoptimize intrinsic. After giving it some thought, I see no solid reason why we should impose any limitations on deopt block.

I don't see any fundamental flaws in the algorithm, it looks pretty robust. I have some nit comments, otherwise it LGTM. (Note that I'm maybe not the most qualified person to approve changes in such fundamental components as BasicBlock and Instruction, but this change seems profitable).

I tried to reformulate the semantics to make it independent on deoptimize intrinsic.

Fixed formatting.

Rebased on top of D51664. This patch is really helpful here because it makes the invalidation of OrderedInstructions automatic. With that, we *only* need to invalidate whenever we may actually change the first ICF instruction or when we erase a basic block. With that, I've discarded most of invalidation we've added on the previous steps because it is actually not needed. We don't need to invalidate when we insert Phis or do stuff like that.

I've run a big corpus of fuzz tests on this patch and it passed OK. So the patch seems good to me in terms of stability. Unfortunately I don't have time to give a proper code review on that. :(

Abandoning in favor of https://reviews.llvm.org/D51664

Thanks @fhahn , I haven't seen it before. Actually after studying the code I came to the same conclusions as they did (that we only need to invalidate when we insert instructions or remove blocks). The mental problem I have is the comment for this class stating that: "A OrderedBasicBlock instance should be discarded whenever the source BasicBlock changes". This, in particular, means that I can check that we don't have dangling pointers whenever we make a request to this class...

https://reviews.llvm.org/D51664 This patch claims to make the OrderedInstructions auto-invalidable. If we build on top of that (and if it works :)), we may give up all invalidation stuff we currently have and only invalidate when we remove ICF instructions from blocks.

After digging deeper into the code, I grow more and more convinced that there was no bug to start with, and dangling pointers to instructions that are not tracked by ICF's map (even if they are tracked by OrderedInstructions) cause no problems.

In D51777#1228447, @skatkov wrote:

I see that IndVarsSimplify uses class field Changed. You propose to use Changed as return value. I just want to see the same approach through the whole IndVars...

Do you plan to modify other pieces to return Changed or may be it makes sense to re-use the class field approach here as well?
What do you think?

Fixed LangRef, renamed pass to "MakeGuardsExplicit".

This patch has been reverted, the situation it protects against seems still possible. It means that the initial theory was true and we need a unit test on that.

It seems we were wrong: http://lab.llvm.org:8011/builders/clang-ppc64be-linux-multistage/builds/11583/steps/build%20stage%202/logs/stdio

This commit has caused massive buildbot failures on compilation, see for example http://lab.llvm.org:8011/builders/clang-cmake-x86_64-avx2-linux/builds/5953/steps/build%20stage%201/logs/stdio

I've just checked that this test now works fine. Apparently it was fixed somehow else, and I cannot construct another test where it matters. So let's just abandon it, not clear if it's needed anymore or not.

After offline discussion with Philip, we agreed on the following version: by default, we have one-block verification to keep compile time in reasonable bounds. For troubleshooting purposes, there is an option (false by default and with no intention to turn it on) to diagnose it. Assertion on destructor does not happen because we don't have a commitment to keep the information valid after the last point where the last query was made.

Added clarification comment, updated commit message to describe what is going on.

Now this is an NFC. We will visit every place where explicit guards need enabling independently.

Need to add tests on passes that use isGuard to see how they feel about it. Needs rework.

Fixed analysis preservation.

Fixed comments, added bundles to tests.

In D51286#1222501, @mkazantsev wrote:

In D51286#1222271, @etherzhhb wrote:

Is it possible to fix the problem by directly codegening the constant from SCEVConstant in expandSCEVIfNeeded?

What's the point of that? I generally don't see any profit in going manually into rewriting internals and trying to find something in its map. Expand methods are designed to handle such cases.