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

[Reid Spencer]

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).

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