[llvm-commits] [llvm] r51461 - in /llvm/trunk: include/llvm/Instructions.h lib/VMCore/Constants.cpp lib/VMCore/Instructions.cpp

[Dan Gohman]

On May 29, 2008, at 8:35 AM, Matthijs Kooijman wrote:

> Hi Dan,
>
>> =
>> =
>> =
>> =
>> =
>> =
>> =
>> =
>> =
>> =====================================================================
>> --- llvm/trunk/lib/VMCore/Instructions.cpp (original)
>> +++ llvm/trunk/lib/VMCore/Instructions.cpp Thu May 22 19:36:11 2008
>> @@ -1333,9 +1333,69 @@
> [...]
>> +void InsertValueInst::init(Value *Agg, Value *Val, Value* const  
>> *Idx, unsigned NumIdx) {
>> +  assert(NumOperands == 1+NumIdx && "NumOperands not initialized?");
>> +  Use *OL = OperandList;
>> +  OL[0].init(Agg, this);
>> +  OL[1].init(Val, this);
>> +
>> +  for (unsigned i = 0; i != NumIdx; ++i)
>> +    OL[i+2].init(Idx[i], this);
>> +}
> From the looks of this, that assert should say 2+NumIdx, the 1 seems  
> copied
> from ExtractValueInst. Or am I missing something here?

You're right. Good catch.

>
>
> Also, in an earlier revision:
>> New Revision: 51157
>
> In Instructions.h:
>> +  template<typename InputIterator>
>> +  static InsertValueInst *Create(Value *Agg, Value *Val,  
>> InputIterator IdxBegin,
>> +                                 InputIterator IdxEnd,
>> +                                 const std::string &Name = "",
>> +                                 Instruction *InsertBefore = 0) {
>> +    typename std::iterator_traits<InputIterator>::difference_type  
>> Values =
>> +      1 + std::distance(IdxBegin, IdxEnd);
>> +    return new(Values)
>> +      InsertValueInst(Agg, Val, IdxBegin, IdxEnd, Values, Name,  
>> InsertBefore);
>> +  }
>> +  template<typename InputIterator>
>> +  static InsertValueInst *Create(Value *Agg, Value *Val,
>> +                                 InputIterator IdxBegin,  
>> InputIterator IdxEnd,
>> +                                 const std::string &Name,
>> +                                 BasicBlock *InsertAtEnd) {
>> +    typename std::iterator_traits<InputIterator>::difference_type  
>> Values =
>> +      1 + std::distance(IdxBegin, IdxEnd);
>> +    return new(Values)
>> +      InsertValueInst(Agg, Val, IdxBegin, IdxEnd, Values, Name,  
>> InsertAtEnd);
>> +  }
> Shouldn't this be 2 + as well on both constructors?

Yep.

>
>
>> +  /// Constructors - These two constructors are convenience  
>> methods because one
>> +  /// and two index insertvalue instructions are so common.
>> +  InsertValueInst(Value *Agg, Value *Val,
>> +                  Value *Idx, const std::string &Name = "",
>> +                  Instruction *InsertBefore = 0);
>> +  InsertValueInst(Value *Agg, Value *Val, Value *Idx,
>> +                  const std::string &Name, BasicBlock *InsertAtEnd);
>
> It seems these two were declared but not implemented, I will probably
> implement these now or tomorrow, unless you do it first :-)

Thanks, however I'm actually in the middle of making some significant
changes to this code. Following on review feedback, I'm going to change
it to use compile-time constant indicies (uint64_t) instead of
Constant*'s, and make some other changes that go along with that.

That change means ExtractValue and InsertValue no longer need
variable-length Use lists, and I'm still working out how things
ought to look under Gabor's new framework (it *is* saving us memory
at least, right? :-})

Dan