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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
