Hi all, any comments about this patch? I'd like to get it out of my working copy :-) Gr. Matthijs> Index: lib/VMCore/Type.cpp > ==================================================================> --- lib/VMCore/Type.cpp (revision 53136) > +++ lib/VMCore/Type.cpp (working copy) > @@ -394,9 +394,8 @@ > > bool StructType::indexValid(const Value *V) const { > // Structure indexes require 32-bit integer constants. > - if (V->getType() == Type::Int32Ty) > - if (const ConstantInt *CU = dyn_cast<ConstantInt>(V)) > - return indexValid(CU->getZExtValue()); > + if (const ConstantInt *CU = dyn_cast<ConstantInt>(V)) > + return indexValid(CU->getZExtValue()); > return false; > } > > @@ -1530,9 +1529,8 @@ > } > > bool SequentialType::indexValid(const Value *V) const { > - if (const IntegerType *IT = dyn_cast<IntegerType>(V->getType())) > - return IT->getBitWidth() == 32 || IT->getBitWidth() == 64; > - return false; > + // Any integer value is allowed > + return V->getType()->isInteger(); > } > > namespace llvm { > Index: lib/Target/TargetData.cpp > ==================================================================> --- lib/Target/TargetData.cpp (revision 53136) > +++ lib/Target/TargetData.cpp (working copy) > @@ -553,8 +553,7 @@ > TI = gep_type_begin(ptrTy, Indices, Indices+NumIndices); > for (unsigned CurIDX = 0; CurIDX != NumIndices; ++CurIDX, ++TI) { > if (const StructType *STy = dyn_cast<StructType>(*TI)) { > - assert(Indices[CurIDX]->getType() == Type::Int32Ty && > - "Illegal struct idx"); > + assert(isa<ConstantInt>(Indices[CurIDX]) && "Illegal struct idx"); > unsigned FieldNo = cast<ConstantInt>(Indices[CurIDX])->getZExtValue(); > > // Get structure layout information...-------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 189 bytes Desc: Digital signature URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20080710/b7db6765/attachment.sig>
I don't think this is right. According to llvm documentation: The index types specified for the 'getelementptr' instruction depend on the pointer type that is being indexed into. Pointer and array types can use a 32-bit or 64-bit integer type but the value will always be sign extended to 64-bits. Structure and packed structure types require i32 constants. Evan On Jul 10, 2008, at 3:47 AM, Matthijs Kooijman wrote:> Hi all, > > any comments about this patch? I'd like to get it out of my working > copy :-) > > Gr. > > Matthijs > >> Index: lib/VMCore/Type.cpp >> ==================================================================>> --- lib/VMCore/Type.cpp (revision 53136) >> +++ lib/VMCore/Type.cpp (working copy) >> @@ -394,9 +394,8 @@ >> >> bool StructType::indexValid(const Value *V) const { >> // Structure indexes require 32-bit integer constants. >> - if (V->getType() == Type::Int32Ty) >> - if (const ConstantInt *CU = dyn_cast<ConstantInt>(V)) >> - return indexValid(CU->getZExtValue()); >> + if (const ConstantInt *CU = dyn_cast<ConstantInt>(V)) >> + return indexValid(CU->getZExtValue()); >> return false; >> } >> >> @@ -1530,9 +1529,8 @@ >> } >> >> bool SequentialType::indexValid(const Value *V) const { >> - if (const IntegerType *IT = dyn_cast<IntegerType>(V->getType())) >> - return IT->getBitWidth() == 32 || IT->getBitWidth() == 64; >> - return false; >> + // Any integer value is allowed >> + return V->getType()->isInteger(); >> } >> >> namespace llvm { >> Index: lib/Target/TargetData.cpp >> ==================================================================>> --- lib/Target/TargetData.cpp (revision 53136) >> +++ lib/Target/TargetData.cpp (working copy) >> @@ -553,8 +553,7 @@ >> TI = gep_type_begin(ptrTy, Indices, Indices+NumIndices); >> for (unsigned CurIDX = 0; CurIDX != NumIndices; ++CurIDX, ++TI) { >> if (const StructType *STy = dyn_cast<StructType>(*TI)) { >> - assert(Indices[CurIDX]->getType() == Type::Int32Ty && >> - "Illegal struct idx"); >> + assert(isa<ConstantInt>(Indices[CurIDX]) && "Illegal struct >> idx"); >> unsigned FieldNo = cast<ConstantInt>(Indices[CurIDX])- >> >getZExtValue(); >> >> // Get structure layout information... > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20080710/50f14a5a/attachment.html>
Hi Evan,> I don't think this is right. According to llvm documentation: > > The index types specified for the 'getelementptr' instruction depend on the > pointer type that is being indexed into. Pointer and array types can use a > 32-bit or 64-bit integer type but the value will always be sign extended to > 64-bits. Structure and packed structure types require i32 constants.That's correct. I'm not saying to allow other integer types for GEP instructions. The first two methods below are methods in StructType and SequentialType. Regardless of any limitations posed by a GEP instruction, I argue that "i8 2" is a perfectly valid index for a struct. The fact that it is not valid for a GEP instruction, should be checked somewhere else (GEPInst constructor or GEPInst::getIndexedValue probably). The last method in the patch is directly related to GEP indices, so asserting only the constness might not be enough, though it would not hurt to make this assert slightly more general (ie, don't check the bitwidth as well). Gr. Matthijs -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 189 bytes Desc: Digital signature URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20080710/7ee7033c/attachment.sig>