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

[Philip Reames via Phabricator via llvm-commits]

reames added inline comments.


================
Comment at: llvm/lib/Analysis/InstructionPrecedenceTracking.cpp:96
+    if (&I == It->second)
+      // No special instruction before cached result
       return;
----------------
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.  


================
Comment at: llvm/lib/Analysis/InstructionPrecedenceTracking.cpp:128
+      FirstSpecialInsts[BB] = &*std::next(Inst->getIterator());
+  }
 }
----------------
mkazantsev wrote:
> This looks correct, but it's not entirely an NFC. We used to completely drop cache, assuming that after the removal we could insert smth in the beginning of this block and this will be fine. Now, if we insert a special instruction before this point, it will not be found. Could you please review the users of `removeInstruction` and confirm that we never use it as invalidation for insertion (which we shouldn't be doing, but not a fact that we don't).
This was never the documented public interface of this class, but in theory it's a risk.  I have glanced at existing usage, and didn't see anything suspicious.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111768