llvm dev - Mar 2009 - [LLVMdev] [Bug 3756] __attribute__((always_inline)) and __builtin_constant_p

On Mar 15, 2009, at 6:16 PM, Nick Lewycky wrote:
> Pierre Habouzit wrote:
>> [ please CC: me as I'm not subscribed ]
>>
>> On Wed, Mar 11, 2009 at 04:13:34AM +0000, bugzilla- 
>> daemon at cs.uiuc.edu wrote:
>>> http://llvm.org/bugs/show_bug.cgi?id=3756
>>>
>>> Chris Lattner <clattner at apple.com> changed:
>>>
>>>           What    |Removed                     |Added
>>>
----------------------------------------------------------------------------
>>>             Status|NEW                         |RESOLVED
>>>         Resolution|                            |WONTFIX
>>>
>>> --- Comment #6 from Chris Lattner <clattner at apple.com> 
2009-03-10
>>> 23:13:33 ---
>>> Unfortunately, this will never be fixed in either llvm-gcc or
clang.
>>> __builtin_constant_p is a "best effort" constant folding
tester,
>>> which is
>>> allowed to fail.  You should never write code that assumes that
>>> __builtin_constant_p can't fail (if you need that, just
don't use
>>> __builtin_constant_p).
>>
>>
>>> It would be interesting and useful to bring this up on the glibc  
>>> list
>>> to see if they are willing to fix their headers.
>>
>> Hahahaha, you never talked to Uli before did you ?
>>
>>> Barring that, if this is really important to you, please raise the
>>> issue on the
>>> llvmdev list.  We will need IR extensions to support this among  
>>> other things.
>>
>> Well, I don't expect __builtin_constant_p to works always.
That's not
>> precisely the issue. I expect though __attribute__((always_inline))
>> functions to be expanded prior to any optimization pass, because
it's
>> what it means. Those functions are basically type-safe macros.
>>
>> If you do that, then my problem go away at once.
>
> No, that's not the problem.
I disagree; I think incomplete support for always_inline is the  
primary issue
here.
>
> Before any optimizations can be applied the function must be converted
> into LLVM IR. In LLVM IR we have no equivalent for  
> "builtin_constant_p".
> Thus the __builtin_constant_p is evaluated and found false as part of
> converting the GCC trees into LLVM IR, then the always_inliner is
> applied on the IR.
Supporting always_inline as a type-safe macro which is always expanded
early is a feature of GCC that people have found useful.  It is a  
limitation of
LLVM that this happens to be complicated to implement there.

Dan

Dan Gohman wrote:
> On Mar 15, 2009, at 6:16 PM, Nick Lewycky wrote:
> 
>> Pierre Habouzit wrote:
>>> [ please CC: me as I'm not subscribed ]
>>>
>>> On Wed, Mar 11, 2009 at 04:13:34AM +0000, bugzilla- 
>>> daemon at cs.uiuc.edu wrote:
>>>> http://llvm.org/bugs/show_bug.cgi?id=3756
>>>>
>>>> Chris Lattner <clattner at apple.com> changed:
>>>>
>>>>           What    |Removed                     |Added
>>>>
----------------------------------------------------------------------------
>>>>             Status|NEW                         |RESOLVED
>>>>         Resolution|                            |WONTFIX
>>>>
>>>> --- Comment #6 from Chris Lattner <clattner at apple.com>
2009-03-10
>>>> 23:13:33 ---
>>>> Unfortunately, this will never be fixed in either llvm-gcc or
clang.
>>>> __builtin_constant_p is a "best effort" constant
folding tester,
>>>> which is
>>>> allowed to fail.  You should never write code that assumes that
>>>> __builtin_constant_p can't fail (if you need that, just
don't use
>>>> __builtin_constant_p).
>>>
>>>> It would be interesting and useful to bring this up on the
glibc
>>>> list
>>>> to see if they are willing to fix their headers.
>>> Hahahaha, you never talked to Uli before did you ?
>>>
>>>> Barring that, if this is really important to you, please raise
the
>>>> issue on the
>>>> llvmdev list.  We will need IR extensions to support this among
>>>> other things.
>>> Well, I don't expect __builtin_constant_p to works always.
That's not
>>> precisely the issue. I expect though __attribute__((always_inline))
>>> functions to be expanded prior to any optimization pass, because
it's
>>> what it means. Those functions are basically type-safe macros.
>>>
>>> If you do that, then my problem go away at once.
>> No, that's not the problem.
> 
> I disagree; I think incomplete support for always_inline is the  
> primary issue
> here.
> 
>> Before any optimizations can be applied the function must be converted
>> into LLVM IR. In LLVM IR we have no equivalent for  
>> "builtin_constant_p".
>> Thus the __builtin_constant_p is evaluated and found false as part of
>> converting the GCC trees into LLVM IR, then the always_inliner is
>> applied on the IR.
> 
> Supporting always_inline as a type-safe macro which is always expanded
> early is a feature of GCC that people have found useful.  It is a  
> limitation of
> LLVM that this happens to be complicated to implement there.
Okay. I find it hard to call this a bug in LLVM's always-inliner, as 
there's nothing you can change in the always-inliner to fix it.

Are you suggesting that we support this by running GCC's always-inliner? 
I don't think that fixes the example unless you also run some GCC 
optimizations to perform the forward substitution. That's a problem when 
we're trying to run the LLVM optimizers instead.

The most straight-forward way to implement it is to leave this function 
in the code, and write a pass that detects a call to this magic function 
and does isa<Constant> on the parameter and replaces the call with the 
result. That pass sounds specific enough to this GCC feature that it 
should probably just live in the llvm-gcc tree.

Thoughts?

Nick

On Thu, Mar 19, 2009 at 11:57 PM, Nick Lewycky <nicholas at mxc.ca>
wrote:
> That pass sounds specific enough to this GCC feature that it
> should probably just live in the llvm-gcc tree.
clang is also going to want any solution here... although, since such
a pass would only be about 10 lines of code, it might not be a big
deal to duplicate it.

-Eli

On Mar 19, 2009, at 11:57 PM, Nick Lewycky wrote:
> Dan Gohman wrote:
>> On Mar 15, 2009, at 6:16 PM, Nick Lewycky wrote:
>>> Before any optimizations can be applied the function must be  
>>> converted
>>> into LLVM IR. In LLVM IR we have no equivalent for
>>> "builtin_constant_p".
>>> Thus the __builtin_constant_p is evaluated and found false as part
>>> of
>>> converting the GCC trees into LLVM IR, then the always_inliner is
>>> applied on the IR.
>>
>> Supporting always_inline as a type-safe macro which is always  
>> expanded
>> early is a feature of GCC that people have found useful.  It is a
>> limitation of
>> LLVM that this happens to be complicated to implement there.
>
> Okay. I find it hard to call this a bug in LLVM's always-inliner, as
> there's nothing you can change in the always-inliner to fix it.
It's a missing feature in llvm-gcc, from an end-user perspective.
How it might be implemented is an implementation detail.

Dan