On May 22, 2007, at 10:41 PM, Reid Spencer wrote: > On Tue, 22 May 2007 22:12:17 -0700 > Chris Lattner <[EMAIL PROTECTED]> 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@cs.uiuc.edu >> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits > > _______________________________________________ > llvm-commits mailing list > llvm-commits@cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits _______________________________________________ llvm-commits mailing list llvm-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits