[PATCH] D42179: [NewGVN] Re-evaluate phi of ops after moving an instr to new class

[Daniel Berlin via Phabricator via llvm-commits]

dberlin added a comment.

To try to help explain this (and feel free to turn it into a comment in NewGVN), for a phi of ops, let me try to cover the cases.

We are trying to transform operations on phis into phis of existing operations.

IE add phi(a, b) 5
into
phi(existing add a, 5, existing add b, 5)

there are essentially two main cases:

1. We found existing-in-the-program operands for all cases (or they are constants).  In this case, we need to make sure that when those operands change congruence class, we re-evaluate the phi of ops.  It should not be needed for the phi of ops to be re-evaluated otherwise (because it only depends on those operands being valid members of their congruence classes).

2. We did not find existing-in-the-program operands for all cases.   In this case, we need to make sure that "if something changes where we might find them, we re-evaluate".

In this case, we mark, symbolically, what expression we were looking for that was missing.  If the expression appears, we should re-evaluate the phi of ops.

Now, maybe you can think of a case i've missed here. Awesome. I don't claim the invariants are perfect, and it definitely requires a lot of thought to get right, so it's entirely possible i'm wrong :)

The code we have is attempting to maintain these invariants.
It is very easy to just over-mark things and make the bugs go away, but eventually it will just hit more bugs (or make it super slow). In particular, zhendong's fuzzer is amazingly good at sussing out fixes that are bandaids, and always seems to find the corner case we miss :)
Given that, historically, when we've hit similar problems, our solution has been to write verifiers.  This is why you have the "value changed after loop completed" verifier (Old GVN cannot survive that verifier, for reference, because re-running analysis definitely changes results. We're trying to do better).

It may be worth writing a phi of ops invariant verifier and see what shakes out.  I certainly don't claim this code is perfect, i just want us to push it in the direction of understanding the invariants we need to maintain, maintaining those invariants and verifying we are doing that.

I usually try to think through it a bit, then give up and write something to just make the contract we are trying to keep explicit and verify it.


Repository:
  rL LLVM

https://reviews.llvm.org/D42179