[llvm-commits] [llvm-gcc] Intrinsics.patch (please apply)

[Chris Lattner]

On May 22, 2007, at 10:41 PM, Reid Spencer wrote:

> On Tue, 22 May 2007 22:12:17 -0700
>  Chris Lattner <clattner at apple.com> wrote:
>>
>> On May 22, 2007, at 12:33 PM, Reid Spencer wrote:
>>
>>> The attached patch contains a small modification to get the index
>>> of the
>>> iAny types correct for overloaded intrinsics. It has been tested and
>>> passes DejaGnu and MultiSource. Please apply.
>>
>> Can you please explain what bad case this (and the previous patch)
>> fixes?  Is this a .ll/.bc syntax change?
>
> No, its not a .ll/.bc syntax change, but it is a slight API  
> correction. The issue has to do with the Intrinsic::getDeclaration  
> method and the interpretation of the (often defaulted) 3rd and 4th  
> parameters that specify the actual types to "fill" in for the iAny  
> parameters (or result).  The third argument is an array of Type*  
> and the fourth is the number of elements.  Previously you had to  
> specify an entry in the array for every parameter and the result  
> type, regardless of whether they were iAny or not. This is  
> inconsistent with uses of Intrinsic::getDeclaration when there are  
> no iAny parameters in which case it is permissable to pass 0 for  
> both the 3rd and 4th arguments (and those happen to the be the  
> default values).

Makes sense, thanks for the clarification reid!

-Chris

> For example, cttz and friends take an iAny parameter but return  
> i32. Consequently they require only one type in the "Tys" argument  
> to getDeclaration (for the parameter). Previously tblgen was  
> indexing into "Tys" using the parameter index (or 0 for the result  
> type) but this leads to "holes" (zero valued Type*) in the array.  
> The interface was changed to eliminate these holes so that the only  
> things passed in Tys is the actual types for the iAny arguments.
>
> Consider this, from LangRef.html:
>
> declare i32 @llvm.ctpop.i8 (i8  <src>)
>
> The single suffix (.i8) that qualifies the argument type indicates  
> that you should be able to call getDeclaration with:
>
> Type *Tys[] = { Type::Int8Ty };
> Function *F = Intrinsics::getDeclaration(Mod, Intrinsics::ctpop,  
> Tys, 1);
>
> However, such a call previously caused an assertion (or worse). The  
> problem is that the code generated by tblgen was indexing into Tys  
> with a subscript of 1 (to match the first argument) but the array  
> was only 1 item long. Consequently, you'd read junk from the stack  
> and assert (or worse).
>
> Previously one would have had to pass, to getDeclaration, something  
> like:
>
> Type *Tys[] = { 0, Type::Int8Ty };
> Function *F = Intrinsics::getDeclaration(Mod, Intrinsics::ctpop,  
> Tys, 2);
>
> which is counter-intuitive since you don't have to use the 3rd and  
> 4th parameters at all (defaulting them to 0) if there are no iAny  
> arguments. The function changed in the llvm-gcc patch was doing  
> exactly this (an array of 2 with a 0 first element) and I changed  
> it to use the former example (single element array).
>
> This was noticed by AutoESL when they tried to call getDeclaration  
> directly themselves and ended up getting assertions and verifier  
> failures.  This wasn't noticed before in LLVM  because the only use  
> of an overloaded intrinsic in LLVM is bswap and that one is a  
> degenerate case.  Both its argument and its result are iAny so you  
> end up needing an array that has two elements anyway - one for the  
> result, one for the argument. However, in cases like part_select,  
> part_set, cttz, ctlz and ctpop, that is not the case.
>
>>
>> -Chris
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits