[PATCH] D74943: [GISel][KnownBits]{NFC} Add a cache mechanism to speed compile time

Fri Feb 21 14:57:45 PST 2020

dsanders added a comment.

I've spoken to Quentin offline and it seems we're not in as much disagreement as it appears. The main issues are:

1. Non-SSA passes aren't included in the plan (I'm focused on IRTranslator to InstructionSelector which are all SSA)
2. Detecting when the contract is violated
3. Not having to write maintenance code

I'll give some more thought to those before I (eventually) make the proposal. I think I have 3. covered, 2. is broadly ok but certainly has room for improvement. For 1. I suggested an off-the-cuff solution that abstracted the cache key to allow implementations to compute a value number by some means instead of directly using the vreg number but it needs some proper thought.

Quentin also mentioned that we should be able to take information from the frontend and I agree with that.

@qcolombet: Does that match your view of our conversation?

In D74943#1887302 <https://reviews.llvm.org/D74943#1887302>, @qcolombet wrote:

> > I think the argument that because it's possible to do the wrong thing if you try hard enough that you have to protect against that is rather flawed and inconsistent with the existing code in LLVM.
>
> I disagree. We never said or enforced that virtual registers are not reused

It's true that we've never said or enforced that. However, for Legalizer, RegBankSelect, Combine, and InstructionSelect it's what's been done in practice as a result of what's needed to achieve correctness for these passes. I do agree that we need to be able to catch when somebody did something weird here. That said, personally I don't feel we need to account for things like pointlessly shuffling vreg numbers. If we can catch that too with a perfect verifier then that's great and makes us very confident but I feel the required scope of the verifier (particularly in the context of a partial known-bits implementation) should be programming mistakes rather than things that would only arise due to intentional attacks on the algorithm.

Part of the eventual proposal will be that so long as you follow that additional constraint, you will get maintenance free preservation of the known bits analysis. If you don't follow that additional constraint then you must either not preserve the analysis, purge affected vregs from the cache, or repair the information. Effectively you opt in to a requirement to either follow an additional constraint, or repair/purge data that didn't follow it. The default is for known-bits to own the maintenance burden which would be done in a very conservative manner, being limited to caching within the getKnownBits() call but not between them and that the pass manager destroys any data left after the pass.

> Your example with SSA is different: this is a well defined, checked and enforced property, unlike the virtual register property you want.
> 
> Even if we were to decide that yes, virtual registers must not change values overtime (while in SSA form), this is not something we could check or enforce, therefore we open ourselves to hard to track bugs. To me that is a design flaw.

The verifier I had in mind can check and enforce direct contradictions (known bits flipping to the other value, or simultaneously claiming to be both values). It's not able to check things the analysis has newly learned (unknown bits becoming known) through new code being more analyzable by being implemented or by being within the search depth. Similarly it can't check things that the analysis has forgotten (known bits becoming unknown) through an incomplete implementation or being outside the search depth. The search depth issue is less of a problem in the context of caching though as the cache permits a much larger search as information can transfer from one query to another even if different instructions are queried.

In addition to the verifier, I want to be able to use analysis serialization to detect behaviour differences arising from a preserved analysis compared to a fresh one. These differences don't necessarily mean that there's a bug, it merely allows us to test the preserved case which isn't possible with `-stop-before`/`-start-before`/`-run-pass` today

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74943/new/

https://reviews.llvm.org/D74943