Page MenuHomePhabricator

Use is.constant intrinsic for __builtin_constant_p
ClosedPublic

Authored by void on Nov 9 2018, 2:08 PM.

Details

Summary

A __builtin_constant_p may end up with a constant after inlining. Use
the is.constant intrinsic if it's a variable that's in a context where
it may resolve to a constant, e.g., an argument to a function after
inlining.

Diff Detail

Repository
rC Clang

Event Timeline

void created this revision.Nov 9 2018, 2:08 PM
void updated this revision to Diff 173445.Nov 9 2018, 2:18 PM

Adding ConstantExpr visitor.

void added a comment.Nov 9 2018, 2:23 PM

This has D54356 integrated into it. D54356 should be reviewed and submitted first, even though it's out of order.

rsmith added inline comments.Nov 13 2018, 12:36 PM
include/clang/AST/Expr.h
908–912

If we're creating two ConstantExpr wrappers for the same expression, that seems like a bug in the caller to me. When does this happen?

lib/AST/Expr.cpp
2924–2925

Can we just return true here? If not, please add a FIXME saying that we should be able to.

lib/Sema/SemaExpr.cpp
4991–4992

We should create this node as part of checking that the argument is an ICE (in SemaBuiltinConstantArg).

14171–14172

I don't understand why CurContext matters here. Can you explain the intent of this check?

void updated this revision to Diff 173922.Nov 13 2018, 1:09 PM
void marked an inline comment as done.

Remove some extraneous checks.

void added inline comments.Nov 13 2018, 1:11 PM
include/clang/AST/Expr.h
908–912

I saw a place that was trying to do that, but it may no longer be valid. I can remove the check for now.

lib/AST/Expr.cpp
2924–2925

To be honest, I don't know. Theoretically we should be able to. Let me give it a try and see how it goes.

lib/Sema/SemaExpr.cpp
14171–14172

It may have been an artifact of development. I can remove it.

void marked an inline comment as not done.Nov 13 2018, 2:50 PM
void added inline comments.
lib/AST/Expr.cpp
2924–2925

I tested this and it looks like it could lead to an extra warning like this:

File /sandbox/morbo/llvm/llvm-mirror/tools/clang/test/Sema/array-init.c Line 285: cannot initialize array of type 'int [5]' with non-constant array of type 'int [5]'

(Similar in Sema/compound-literal.c.) I'll add a comment.

lib/Sema/SemaExpr.cpp
4991–4992

It turns out that SemaBuiltinConstantArg isn't called for __builtin_constant_p(). Its argument type is ., which...I'm not sure what that means. It looks like a vararg, but that doesn't make sense for the builtin.

Should I change its signature in Builtins.def?

rsmith added inline comments.Nov 13 2018, 3:59 PM
lib/CodeGen/CGBuiltin.cpp
1931–1932

Are there cases where a call to an always_inline function could change the outcome?

lib/Sema/SemaExpr.cpp
4991–4992

It's also marked t, which means "signature is meaningless, uses custom type checking".

The argument to __builtin_constant_p isn't required to be an ICE, though, so we shouldn't be calling SemaBuiltinConstantArg for it / creating a ConstantExpr wrapping it, as far as I can tell.

void updated this revision to Diff 173970.Nov 13 2018, 5:56 PM
void marked an inline comment as not done.

Updated to address comments.

void added a comment.Nov 13 2018, 5:58 PM

I think this is ready now. PTAL.

lib/CodeGen/CGBuiltin.cpp
1931–1932

Yes, but the result of bcp isn't guaranteed to be the same for optimized and unoptimized code. GCC's documentation says that it won't return 1 unless -O is used. Their code appears to back this up somewhat:

switch (fcode)
  {
  case BUILT_IN_CONSTANT_P:
    {
      tree val = fold_builtin_constant_p (arg0);

      /* Gimplification will pull the CALL_EXPR for the builtin out of
         an if condition.  When not optimizing, we'll not CSE it back.
         To avoid link error types of regressions, return false now.  */
      if (!val && !optimize)
        val = integer_zero_node;

      return val;
    }
  ...
  }

Our inlining is different of course, and I'm not sure when GCC performs their always_inline inlining. It may be before the above code is called. But I think we're within reasonable bounds with respect to the documentation.

(That said, GCC has a tendency to violate its own documentation. So caveat emptor, etc.)

lib/Sema/SemaExpr.cpp
4991–4992

Right. I think I was looking at something else. I changed it.

void updated this revision to Diff 173974.Nov 13 2018, 6:25 PM

Fix overflow flag.

void updated this revision to Diff 173986.Nov 13 2018, 9:48 PM

ImpCast.

void added a comment.Nov 18 2018, 1:45 AM

Ping? I really don't want this review to go on forever.

void updated this revision to Diff 174529.Nov 18 2018, 1:57 AM

Don't re-wrap a ConstExpr.

void updated this revision to Diff 174551.Nov 18 2018, 5:32 PM

No function pointers

Looks good other than the ODR violation issue.

include/clang/AST/Expr.h
37–39

It's not appropriate to declare internal-linkage types in headers. This will create ODR violations, and appears to be unnecessary -- the extra EvaluateAsRValue overload you declare below is not callable outside of ExprConstant.cpp, so can instead be declared as an internal linkage function in that file.

void updated this revision to Diff 174692.Nov 19 2018, 2:53 PM

Remove ODR violation.

void marked an inline comment as done.Nov 19 2018, 2:54 PM
void added a comment.Nov 19 2018, 3:20 PM

Should be okay now. PTAL. :-)

lib/StaticAnalyzer/Core/ExprEngine.cpp
1285–1286

LLVM_FALLTHROUGH; ?

lib/StaticAnalyzer/Core/ExprEngine.cpp
1285–1286

nvm, only needed when there's code THEN fallthrough.

This revision was not accepted when it landed; it landed in state Needs Review.Nov 20 2018, 12:56 AM
Closed by commit rC347294: Use is.constant intrinsic for __builtin_constant_p (authored by void, committed by ). · Explain Why
This revision was automatically updated to reflect the committed changes.

It seems this patch caused the SystemZ build bots to fail, they're now all running into assertion failures:

ICE cannot be evaluated!
UNREACHABLE executed at /home/uweigand/sandbox/buildbot/clang-s390x-linux/llvm/tools/clang/lib/AST/ExprConstant.cpp:11442!

See e.g. http://lab.llvm.org:8011/builders/clang-s390x-linux/builds/20645/steps/ninja%20check%201/logs/FAIL%3A%20Clang%3A%3Abuiltins-systemz-zvector-error.c

lib/AST/ExprConstant.cpp
11391

@void , I assume this unreachable is the one reported by @uweigand ? Does the above switch need a case statement added?

https://reviews.llvm.org/rC347417 makes constexpr string_view service = "HELLO WORD SERVICE" (P0426) broken with libstdc++

% cat a.cc
constexpr bool __constant_string_p(const char *__s) {
  while (__builtin_constant_p(*__s) && *__s)
    __s++;
  return __builtin_constant_p(*__s);
}

constexpr bool n = __constant_string_p("a");
% fclang++ -std=c++17 -fsyntax-only a.cc
a.cc:1:16: error: constexpr function never produces a constant expression [-Winvalid-constexpr]
constexpr bool __constant_string_p(const char *__s) {
               ^
a.cc:2:10: note: subexpression not valid in a constant expression
  while (__builtin_constant_p(*__s) && *__s)
         ^
a.cc:7:16: error: constexpr variable 'n' must be initialized by a constant expression
constexpr bool n = __constant_string_p("a");
               ^   ~~~~~~~~~~~~~~~~~~~~~~~~
a.cc:2:10: note: subexpression not valid in a constant expression
  while (__builtin_constant_p(*__s) && *__s)
         ^
a.cc:7:20: note: in call to '__constant_string_p(&"a"[0])'
constexpr bool n = __constant_string_p("a");
                   ^
2 errors generated.

Fourth time should be a charm...

void added a comment.Nov 25 2018, 4:28 PM

Doh! I'll take a look at it.

void added a comment.Nov 25 2018, 6:14 PM

Fixed in r347531.

I'm seeing a case where an inline assembly 'n' constraint is behaving differently in -O0 now. Previously we would evaluate the builtin_constant_p as part of the EvaluateAsInt call in CodeGenFunction::EmitAsmInput, but I think we're not doing that now. This causes us to fail later because the constraint wasn't satisfied since we were in -O0 and thus we didn't optimize the rest of the expression. The test case we have is a nasty mix of builtin_constant_p, builtin_choose_expr, and builtin_classify_type. I can see about sharing it if needed.

void added a comment.Dec 10 2018, 9:09 PM

I'm seeing a case where an inline assembly 'n' constraint is behaving differently in -O0 now. Previously we would evaluate the builtin_constant_p as part of the EvaluateAsInt call in CodeGenFunction::EmitAsmInput, but I think we're not doing that now. This causes us to fail later because the constraint wasn't satisfied since we were in -O0 and thus we didn't optimize the rest of the expression. The test case we have is a nasty mix of builtin_constant_p, builtin_choose_expr, and builtin_classify_type. I can see about sharing it if needed.

__builtin_constant_p() is known to have different semantics at -O0 than it does during optimizations. That said, we shouldn't be intentionally generating incorrect code. Could you test it against gcc to see if it has the same semantics? Otherwise, please add a testcase. :-)

Here's the test case that we have https://reviews.llvm.org/P8123 gcc seems to accept it at O0

Here's the test case that we have https://reviews.llvm.org/P8123 gcc seems to accept it at O0

Smaller test case:

extern unsigned long long __sdt_unsp;

void foo() {
  __asm__ __volatile__(
      "" :: "n"( (__builtin_constant_p(__sdt_unsp) || 0) ? 1 : -1));
}
void added a comment.Dec 12 2018, 11:19 AM

The issue is that "n" is expecting an immediate value, but the result of the ternary operator isn't calculated by the front-end, because it doesn't "know" that the evaluation of __builtin_constant_p shouldn't be delayed (it being compiled at -O0). I suspect that this issue will also happen with the "i" constraint.

The issue is that "n" is expecting an immediate value, but the result of the ternary operator isn't calculated by the front-end, because it doesn't "know" that the evaluation of __builtin_constant_p shouldn't be delayed (it being compiled at -O0). I suspect that this issue will also happen with the "i" constraint.

Indeed. We _do_ actually already know that in clang too, however -- clang already knows that "n" requires an immediate, and does constant expression evaluation for it, e.g. to expand constexpr functions. Grep for "requiresImmediateConstant". I guess it's just not hooked up to the __b_c_p correctly, though.

(Note, as a workaround, you could use | instead of ||, or put the expression as the value of an enumeration constant).

void added a comment.Dec 12 2018, 2:12 PM

The issue is that "n" is expecting an immediate value, but the result of the ternary operator isn't calculated by the front-end, because it doesn't "know" that the evaluation of __builtin_constant_p shouldn't be delayed (it being compiled at -O0). I suspect that this issue will also happen with the "i" constraint.

Indeed. We _do_ actually already know that in clang too, however -- clang already knows that "n" requires an immediate, and does constant expression evaluation for it, e.g. to expand constexpr functions. Grep for "requiresImmediateConstant". I guess it's just not hooked up to the __b_c_p correctly, though.

(Note, as a workaround, you could use | instead of ||, or put the expression as the value of an enumeration constant).

I looked at requiresImmediateConstant, but when I set it for n and i things broke all over the place. I sent out a patch for review. It's limited to -O0, since we can compile with optimizations without complaint. And there may be instances where it's necessary:

inline void foo(int x) {
  asm("" : : "n" (__builtin_constant_p(n) ? 1 : -1));
}

void bar() {
  foo(37);
}

That example cannot be expected to ever evaluate the expression as "1" -- it doesn't in GCC, nor should it in Clang. An asm constraint of "n" or "i" (but not, e.g., "nr") must require a constant expression, and evaluating the argument as a constant expression necessarily means always evaluating it to -1.