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