[LLVMdev] [Bug 3756] __attribute__((always_inline)) and __builtin_constant_p

[Nick Lewycky]

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