[PATCH] D82717: [InstSimplify] Fold icmp with dominating assume

[Sanjay Patel via Phabricator via llvm-commits]

spatel accepted this revision.
spatel added a comment.
This revision is now accepted and ready to land.

In D82717#2126613 <https://reviews.llvm.org/D82717#2126613>, @nikic wrote:

> In D82717#2125651 <https://reviews.llvm.org/D82717#2125651>, @spatel wrote:
>
> > Can we do this in InstSimplify instead? (always folds to constant)
>
>
> Done! I have some slight compile-time apprehensions about doing this in InstSimplify. On CTMark this is barely above the noise, but that's probably just a result of little assume use in those benchmarks. But I agree that InstSimplify is logically the right place for this, and we can move it, if it becomes a problem.


Thanks for checking that. Yes, our icmp analysis is known to be expensive, and I remember seeing at least anecdotal evidence that using the assumption cache is also expensive.
So watch out for pushback, but LGTM.



================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:3287
+                                               const SimplifyQuery &Q) {
+  if (!Q.AC || !Q.CxtI)
+    return nullptr;
----------------
Would it make sense to also use "Q.AC.assumptions().empty()" as an early exit?
We use that in InstCombiner::visitSelectInst() to mitigate cost for the presumed common case where there are no assumes available.


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

https://reviews.llvm.org/D82717