[PATCH] D134006: Add an optional cache to computeKnownBits.

[Justin Lebar via Phabricator via llvm-commits]

jlebar added inline comments.


================
Comment at: llvm/include/llvm/Analysis/ValueTracking.h:52
+// We can key lookups on the basic block that contains CxtI, rather than on CxtI
+// itself, because the context instr itself is not significant, only its BB.
+using KnownBitsCache = DenseMap<
----------------
nikic wrote:
> This is incorrect, the context instruction is used to check applicability of assumes for example, see isValidAssumeForContext().
`isGuaranteedToTransferExecutionToSuccessor`, wow, TIL.  Thanks.

But, very sad: With the change to look at the real CtxI rather than its BB, this no longer provides a significant speedup.

This all seems so silly, because in 99.9% of cases the BB terminator really *is* sufficient.

Any thoughts?  Or do you think I should give up on this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134006