[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

[Duncan Sands]

Hi Dan, nice work!

> +/// ExtractValueInst - This instruction extracts a value
> +/// from an aggregate value

I guess you know what I'm going to say here: I think you should
explain that it extracts a field from a struct, an element from an
array.  Plus there's a missing full stop at the end of the comment.

> +  /// Constructors - Create a extractvalue instruction with a base pointer an
> +  /// list of indices.

I don't understand this comment.  Also "an list" -> "a list".

> +  /// A null type is returned if the indices are invalid for the specified
> +  /// pointer type.

I guess by a "null type" you mean a null pointer?

> +/// InsertValueInst - This instruction extracts a value
> +/// from an aggregate value

You know what I'm going to say here too :)  Also, this comment
applies to ExtractValue not InsertValue: needs extract -> insert,
from -> into.

> +  /// Constructors - Create a insertvalue instruction with a base pointer an
> +  /// list of indices.

As above.

> +  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?

> -; 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?

Ciao,

Duncan.