[PATCH] D54355: Use is.constant intrinsic for __builtin_constant_p

[Bill Wendling via Phabricator via cfe-commits]

void added a comment.

I think this is ready now. PTAL.



================
Comment at: lib/CodeGen/CGBuiltin.cpp:1930-1931
+    if (CGM.getCodeGenOpts().OptimizationLevel == 0)
+      // At -O0, we don't perform inlining, so we don't need to delay the
+      // processing.
+      return RValue::get(ConstantInt::get(Int32Ty, 0));
----------------
rsmith wrote:
> Are there cases where a call to an `always_inline` function could change the outcome?
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.)


================
Comment at: lib/Sema/SemaExpr.cpp:4973-4974
 
+    if ((ICEArguments & (1 << (ArgIx - 1))) != 0)
+      Arg = ConstantExpr::Create(Context, Arg);
+
----------------
rsmith wrote:
> void wrote:
> > rsmith wrote:
> > > We should create this node as part of checking that the argument is an ICE (in `SemaBuiltinConstantArg`).
> > 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`?
> 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.
Right. I think I was looking at something else. I changed it.


Repository:
  rC Clang

https://reviews.llvm.org/D54355