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

[Kazu Hirata via Phabricator via llvm-commits]

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

> 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.

We could take a max of the "then" and "else clauses, but that's probably an overkill.  I agree your patch also serves as a reminder about `__builtin_constant_p`.

Please see some nit comments about the test.

LGTM.  Thanks!



================
Comment at: llvm/test/Transforms/Inline/call-intrinsic-is-constant.ll:10
+; CHECK-NEXT:    [[COND:%.*]] = call i1 @llvm.is.constant.i64(i64 [[VAL:%.*]])
+; CHECK-NEXT:    br i1 [[COND]], label [[COND_TRUE:%.*]], label [[COND_FALSE:%.*]]
+; CHECK:       cond.true:
----------------
Nit: I understand that you used `utils/update_test_checks.py` to generate the assertions, but could we hoist out `%` from the regular expression like so?

`%[[COND_TRUE:.*]]`

This way, we can say `[[COND_TRUE]]:` in place of the label.  The same applies to `COND_FALSE`.

Alternatively, you might just drop the assertions for `@callee`.  All the interesting things are happening in `@caller`.


================
Comment at: llvm/test/Transforms/Inline/call-intrinsic-is-constant.ll:11
+; CHECK-NEXT:    br i1 [[COND]], label [[COND_TRUE:%.*]], label [[COND_FALSE:%.*]]
+; CHECK:       cond.true:
+; CHECK-NEXT:    call void @foo()
----------------
`[[COND_TRUE]]:`


================
Comment at: llvm/test/Transforms/Inline/call-intrinsic-is-constant.ll:40
+; CHECK-NEXT:    [[COND_I:%.*]] = call i1 @llvm.is.constant.i64(i64 [[VAL:%.*]])
+; CHECK-NEXT:    br i1 [[COND_I]], label [[COND_TRUE_I:%.*]], label [[COND_FALSE_I:%.*]]
+; CHECK:       cond.true.i:
----------------
The same comment as above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111272