[llvm-commits] [llvm] r51157 - in /llvm/trunk: include/llvm/Constants.h include/llvm/Instruction.def include/llvm/Instructions.h include/llvm/Support/InstVisitor.h lib/AsmParser/llvmAsmParser.cpp.cvs lib/AsmParser/llvmAsmParser.h.cvs lib/AsmParser/llvmAsmParser.y lib/AsmParser/llvmAsmParser.y.cvs lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp lib/VMCore/ConstantFold.cpp lib/VMCore/ConstantFold.h lib/VMCore/Constants.cpp lib/VMCore/Instructions.cpp lib/VMCore/Verifier.cpp test/Verifier/2002-11-05-GetelementptrPointers.ll

[Dan Gohman]

On May 15, 2008, at 1:38 PM, Duncan Sands wrote:
>> +  assert((isa<StructType>(Agg->getType()) || isa<ArrayType>(Agg- 
>> >getType()) ||
>> +          isa<VectorType>(Agg->getType())) &&
>
> Do you want to allow this on VectorType?  Also, how about  
> introducing a
> method for testing for whether a type is an aggregate type?

I don't know yet. It turns out that getelementptr works on vector types,
and that made me think having extractelement/insertelement support them
as well. It would make the IR more concise, because a single  
extractevalue
could be used where otherwise an extractvalue followed by an  
extractelement
would be needed. But it also means instcombine would have to do for
extractvalue/insertvalue what it does for extractelement/insertelement.
And there's no advantage for codegen.

>
>
>> -; This testcase is invalid because we are indexing into a pointer  
>> that is
>> -; contained WITHIN a structure.
>> +; This testcase was previously considered invalid for indexing  
>> into a pointer
>> +; that is contained WITHIN a structure, but this is now valid.
>>
>> define void @test({i32, i32*} * %X) {
>> 	getelementptr {i32, i32*} * %X, i32 0, i32 1, i32 0
>
> Why is this valid now?  Before with GEP with constant indices, you  
> would
> always get a constant offset from the original pointer.  The value  
> only
> depended on the structure of the type, not the actual value of the  
> object.
> But if you are allowing going down into pointers inside structs then  
> this
> is no longer true - the value no longer depends just on the type of  
> %X.
> Or am I misunderstanding?

You're right. I mistook what this test was doing. I'll revert this
test change and fix the new getIndexedType code to reject this.

Thanks,

Dan