Re: [llvm-commits] [llvm-gcc] Intrinsics.patch (please apply)
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
Re: [llvm-commits] [llvm-gcc] Intrinsics.patch (please apply)
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? -Chris ___ llvm-commits mailing list llvm-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
Re: [llvm-commits] [llvm-gcc] Intrinsics.patch (please apply)
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
