[PATCH] D111272: [InlineCost] model calls to llvm.is.constant* more carefully

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added a comment.

In D111272#3049590 <https://reviews.llvm.org/D111272#3049590>, @kazu wrote:

> Hi Nick,
>
> I really like the idea of evaluating `__builin_constant_p`.  If it evaluates to true, we are all good.  Now, if it evaluates to false for now but later evaluates to true after some inlining, the cost may be wrong.  Should we worry about that?  I am guessing that's OK because statements inside the "then" clause of `__builtin_constant_p` tend to fold very well in most real world code.

Correct; that's what the added `FIXME` in the code is alluding to.  While one could manually construct adversarial examples, I don't expect `__builtin_constant_p` to be used in other ways in the wild.  That said, it would be even more precise to peek and see if inlining would change how `__builtin_constant_p` would evaluate.  I'm not sure yet how to do that in LLVM, but this patch is meant to "be less wrong" when `__builtin_constant_p` will NEVER be true regardless of inlining AND add a method where we can explore modeling the cost of this builtin/intrinsic more in the future.

Right now, when the parameter to the builtin/intrinsic is "locally not constant" we compute the cost as if it was a runtime libcall, keeping all possible resulting computations. That's super wrong; it's one, or the other.  This patch picks one, with a FIXME on how to choose the right one even more precisely.

> By the way, could we have a shorter test?  If we just want to see the effect of `__builtin_constant_p` being folded to false, two function definitions should suffice like so:

Thanks, I've adopted your test wholesale.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111272