Hi all,
I'm fiddling around a bit with programmatically created GEP instructions,
which is failing when using i64 for any indix, except the first.
The reason behind this, is that StructureType::indexValid only accepts Value*
that are constant ints of width 32. I can't really see why this limitation is
here.
SequentialType solves this slightly differently, it allows for both 32 and 64
bits integers. However, AFAICS, any constant integer should do just fine, so I
would propose to drop the bitwidth check such that any ConstantInt is valid.
This change should also be made to SequentialType, which can be indexed by any
integer type as well.
I've attached a patch, which makes any integer type valid for indexing an
CompositeType. The side effect is now that GEP no longer poses any conditions
on it's indices (since it delegated that to indexValid, which is not the right
place IMHO). I guess those checks need to explicitely added somewhere in
GetElementPtrInst.
Gr.
Matthijs
indexvalid.diff (924 Bytes)
Hi all,
I just found that TargetData also contains a similar assert, and that I forgot
a "return" in my previous patch.
Here's an updated patch, which allows to use any integer to index a
CompositeType. It seems this enables GEP to work with any integer value as
well, I tested indexing a struct with a i16 an i64 (also through llc). This
should still be adressed somewhere, unless we want to remove the i32 or i64
constraint from GEP.
Gr.
Matthijs
Index: lib/VMCore/Type.cpp
Hi all,
any comments about this patch? I'd like to get it out of my working copy 
Gr.
Matthijs
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
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
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).
That's what the documentation is saying though. Struct and packed structure types require i32 constants. Perhaps I am misunderstanding something?
Evan
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?
-Chris
Hi Chris & Evan,
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