[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 23, 2008, at 2:37 PM, Chris Lattner wrote:

> On May 15, 2008, at 12:50 PM, Dan Gohman wrote:
>> URL: http://llvm.org/viewvc/llvm-project?rev=51157&view=rev
>> Log:
>> IR support for extractvalue and insertvalue instructions. Also, begin
>> moving toward making structs and arrays first-class types.
>
> Ok.
>
>>
>> =
>> =
>> =
>> =
>> =
>> =
>> =
>> =
>> =
>> =====================================================================
>> --- llvm/trunk/include/llvm/Constants.h (original)
>> +++ llvm/trunk/include/llvm/Constants.h Thu May 15 14:50:34 2008
>> @@ -575,6 +575,11 @@
>>                                      Constant *Elt, Constant *Idx);
>>  static Constant *getShuffleVectorTy(const Type *Ty, Constant *V1,
>>                                      Constant *V2, Constant *Mask);
>> +  static Constant *getExtractValueTy(const Type *Ty, Constant *Agg,
>> +                                     Constant * const *Idxs,
>> unsigned NumIdxs);
>> +  static Constant *getInsertValueTy(const Type *Ty, Constant *Agg,
>> +                                    Constant *Val,
>> +                                    Constant * const *Idxs,
>> unsigned NumIdxs);
>
> As discussed in langref, I think this should take just "const unsigned
> *Idxs, unsigned NumIdxs" like getresult.
>
> I understand that this means that you don't get to share code with GEP
> indexing, but it is more efficient and means that we can't have
> constantexprs as indices etc.

Ok.

>
>
>> //
>> =
>> =
>> =
>> ----------------------------------------------------------------------=
>> ==//
>> +//                                ExtractValueInst Class
>> +//
>> =
>> =
>> =
>> ----------------------------------------------------------------------=
>> ==//
>> +
>> +/// ExtractValueInst - This instruction extracts a value
>> +/// from an aggregate value
>> +///
>> +class ExtractValueInst : public Instruction {
>> +  ExtractValueInst(const ExtractValueInst &EVI);
>> +  void init(Value *Agg, Value* const *Idx, unsigned NumIdx);
>> +  void init(Value *Agg, Value *Idx);
>> +
>> +  template<typename InputIterator>
>> +  void init(Value *Agg, InputIterator IdxBegin, InputIterator  
>> IdxEnd,
>> +            const std::string &Name,
>> +            // This argument ensures that we have an iterator we can
>> +            // do arithmetic on in constant time
>> +            std::random_access_iterator_tag) {
>> +    unsigned NumIdx = static_cast<unsigned>(std::distance(IdxBegin,
>> IdxEnd));
>> +
>> +    if (NumIdx > 0) {
>
> I thought the instruction required at least one index?  Likewise in a
> couple other places.

It doesn't really need to require an index. An extractvalue with
no indicies is an identity operation.

>
>
>> +++ llvm/trunk/test/Verifier/2002-11-05-GetelementptrPointers.ll Thu
>> May 15 14:50:34 2008
>> @@ -1,7 +1,7 @@
>> -; RUN: not llvm-as < %s |& grep {Invalid getelementptr indices}
>> +; RUN: llvm-as < %s
>>
>> -; 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?

It isn't. This was a mistake which has since been fixed.

Dan