[cfe-commits] r147655 - in /cfe/trunk: lib/CodeGen/CGBuiltin.cpp test/CodeGen/no-builtin.c

[Richard Smith]

On Fri, January 6, 2012 15:39, David Chisnall wrote:
> Then I am confused - the test case in the bug report failed for me before
> making that change and passed after that change...

Were you running it through FileCheck? It's only checking for 'cos' in the
output, which will naturally exist since you define an extern function with
that name.

> Apparently (according to Ed, who reported PR11711) the part of the regression
> test suite of which this is a reduced form also now passes after this change.
>
> On 6 Jan 2012, at 15:18, Richard Smith wrote:
>> On Fri, January 6, 2012 12:20, David Chisnall wrote:
>>> Author: theraven
>>> Date: Fri Jan  6 06:20:19 2012
>>> New Revision: 147655
>>>
>>>
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=147655&view=rev
>>> Log:
>>> If we are compiling with -fno-builtin then don't do constant folding of
>>> builtins.
>>>
>>> This fixes PR11711.
>>>
>>>
>>>
>>>
>>> Added:
>>> cfe/trunk/test/CodeGen/no-builtin.c Modified:
>>> cfe/trunk/lib/CodeGen/CGBuiltin.cpp
>>>
>>> Modified: cfe/trunk/lib/CodeGen/CGBuiltin.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGBuiltin.cpp?re
>>> v=1 47655&r1=147654&r2=147655&view=diff
>>> ==========================================================================
>>> ====
>>> --- cfe/trunk/lib/CodeGen/CGBuiltin.cpp (original)
>>> +++ cfe/trunk/lib/CodeGen/CGBuiltin.cpp Fri Jan  6 06:20:19 2012
>>> @@ -175,7 +175,8 @@
>>> unsigned BuiltinID, const CallExpr *E) { // See if we can constant fold
>>> this builtin.  If so, don't emit it at all. Expr::EvalResult Result; -  if
>>> (E->EvaluateAsRValue(Result, CGM.getContext()) &&
>>> +  if (!getContext().getLangOptions().NoBuiltin &&
>>> +      E->EvaluateAsRValue(Result, CGM.getContext()) &&
>>> !Result.hasSideEffects()) {
>>> if (Result.Val.isInt()) return
>>> RValue::get(llvm::ConstantInt::get(getLLVMContext(),
>>>
>>
>> I don't understand this change. The constant folder doesn't fold any of the
>>  builtins which the bug report complained about other than fabs. And the
>> constant folder only folds them when they're in the __builtin_ form (which
>> isn't what the PR was complaining about). And this change doesn't prevent
>> the same builtins from being folded in other situations where the constant
>> folder is run.
>>
>>> Added: cfe/trunk/test/CodeGen/no-builtin.c
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/no-builtin.c?re
>>> v=1 47655&view=auto
>>> ==========================================================================
>>> ====
>>> --- cfe/trunk/test/CodeGen/no-builtin.c (added)
>>> +++ cfe/trunk/test/CodeGen/no-builtin.c Fri Jan  6 06:20:19 2012
>>> @@ -0,0 +1,20 @@
>>> +// RUN: %clang_cc1 -fno-builtin -emit-llvm %s -o - | FileCheck %s
>>> +//
>>> +// Check that -fno-builtin prevents us from constant-folding through
>>> builtins +// (PR11711)
>>>
>>
>> I cannot reproduce the problem reported in this bug, before or after your
>> change.
>>
>>> +
>>> +double
>>> +cos(double x)
>>> +{
>>> +  printf("ok\n");
>>> +  exit(0);
>>> +}
>>> +
>>> +int
>>> +main(int argc, char *argv[])
>>> +{
>>> +  cos(1); // CHECK: cos
>>> +  printf("not ok\n");
>>> +  abort();
>>> +}
>>> +
>>>
>>
>> This test passes with or without your change, with or without -fno-builtin.
>>
>>
>
>