Hi Chris & Evan, On Sun, Jul 13, 2008 at 03:34:03PM -0700, Chris Lattner wrote:> Why? What value does that provide? Struct indices are not allowed to > be variable, so picking any width constant (that isn't too small) is > fine. What problem are you trying to solve here?The current code only allows 32 bit constants. My patch allows any width constant to be used, but it seems Evan doesn't agree with that. On Fri, Jul 11, 2008 at 10:56:16AM -0700, Evan Cheng wrote:> >> 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). > That's what the documentation is saying though. Struct and packed > structure types require i32 constants. Perhaps I am misunderstanding > something?Ah, I did miss the "only 32 bit" limitation for structs, which makes my current argpromotion fix wrong. However, I still think that that is a GEP limitation, that does not belong with StructType. Or is there some other piece of documentation you are referring to? Gr. Matthijs
On Jul 15, 2008, at 3:58 AM, Matthijs Kooijman wrote:> Hi Chris & Evan, > > On Sun, Jul 13, 2008 at 03:34:03PM -0700, Chris Lattner wrote: >> Why? What value does that provide? Struct indices are not allowed >> to >> be variable, so picking any width constant (that isn't too small) is >> fine. What problem are you trying to solve here? > The current code only allows 32 bit constants. My patch allows any > width > constant to be used, but it seems Evan doesn't agree with that.Hey Matthijs, I understand what you're trying to do. Why do you want this? What value does it add? Do you need a struct with more than 2^32 fields? -Chris
> I understand what you're trying to do.Ok :-)> Why do you want this? What value does it add? Do you need a struct with > more than 2^32 fields?It's not really needed, but I think it would be more correct. I did run into an issue: I was working on argpromotion (see PR2498), which stores lists of indices. These used to be stored as Value*, but I ran into type problems. For example, when there is a load gep {...} %X, i32 0 in the entry block, and a load gep {...} %X, i64 0 somewhere else, the argument will not be promoted because the second load is supposedly not safe (different indices). I fixed this by replacing the Value* lists with unsigned lists, thus dropping type information (which wasn't really useful anyway). However, when regenerating new GEPs from thos lists, I must choose a type again. To make sure things fit I always used i64, which broke since only i32 was supported. That's where I started fixing things. However, as pointed out by evan, i64 is not valid for indexing a struct (per langref). This means that I should really be emitting GEPs that use an i64 as the first index (since that indexes a pointer) and i32's for all the next indices (which index struct elements). This will make things work within the constraints of GEP again and need any other changes to IndexValid (though I still think that the checks for the operand types of GEP are in the wrong place, but it's not really a problem now). So, problem solved I guess :-) Thanks for the pointers and patience! 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/20080716/78e6f7d9/attachment.sig>