[PATCH] D111768: [IPT] Restructure cache to allow lazy update following invalidation [NFC]

[Philip Reames via Phabricator via llvm-commits]

reames added inline comments.


================
Comment at: llvm/lib/Analysis/InstructionPrecedenceTracking.cpp:52
+    // Seed the lazy scan
+    FirstSpecialInsts[BB] = &*BB->begin();
+
----------------
nikic wrote:
> To avoid doing the BB lookup three times, use something like this?
> ```
> auto It = FirstSpecialInsts.try_emplace(BB, &BB->front()).first;
> Instruction *CurI = It->second;
> // ...
> It->second = Res;
> ```
I'll be honest here and say that I find the try_emplace style code simply unreadable.  I tried to write something based on your suggestion, and gave up when I couldn't decipher the error messages.

I doubt this will be performance critical.  The hot path is the second if, so at most we have two lookups on the fast path.  

I'd strongly prefer to leave this as is.  


================
Comment at: llvm/lib/Analysis/InstructionPrecedenceTracking.cpp:96
+    if (&I == It->second)
+      // No special instruction before cached result
       return;
----------------
mkazantsev wrote:
> reames wrote:
> > mkazantsev wrote:
> > > This loosens the validation. What if  `I` is not special? We used to check this and now we don't.
> > We check to see if I is special just below.  And as indicated in the comment, the cached result does not need to be special, just before any special instruction.  
> If `I` is not special but it somehow got into the `It->second`, where do you detect that?
Max, you appear to be missing the point of the change.  The cached entry (e.g. It->second) is by design no longer always special.  It's simply guaranteed to comeBefore the first special instruction.  It is not an error for the cached instruction not to be special.  It's only an error for there to be a special instruction before the cached one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111768