[llvm-commits] [llvm] r51806 - in /llvm/trunk: docs/LangRef.html include/llvm/Constants.h include/llvm/DerivedTypes.h include/llvm/Instructions.h lib/AsmParser/llvmAsmParser.cpp.cvs lib/AsmParser/llvmAsmParser.h.cvs lib/AsmParser/llvmAsmParser.y lib/AsmParser/llvmAsmParser.y.cvs lib/Bitcode/Reader/BitcodeReader.cpp lib/Bitcode/Writer/BitcodeWriter.cpp lib/VMCore/Constants.cpp lib/VMCore/Instructions.cpp lib/VMCore/Type.cpp test/Assembler/insertextractvalue.ll

[Dan Gohman]

On Sun, June 22, 2008 11:00 am, Chris Lattner wrote:
> On May 30, 2008, at 5:58 PM, Dan Gohman wrote:
>> URL: http://llvm.org/viewvc/llvm-project?rev=51806&view=rev
>> Log:
>> IR, bitcode reader, bitcode writer, and asmparser changes to
>> insertvalue and extractvalue to use constant indices instead of
>> Value* indices. And begin updating LangRef.html.
>
> You guys are making awesome progress,  some thoughts:
>
>
> Are insertvalue/extractvalue valid as constant expressions?  If so,
> langref should be updated and it would be good to see that they encode
> into the bc file correctly.  If we can *guarantee* that they are
> always folded away by the constant folding, then I'm fine with saying
> that they are not allowed as constant exprs.

Currently we should be folding all such constant expressions.

One case where we might not want to is a huge zeroinitializer with a
few non-zero values inserted into it. It's a real situation, though I
don't know if it's enough of a concern to justify here.

>
> In the insertvalue section, you have this syntax:
>
> <result> = insertvalue <aggregate type> <val>, <ty> <val>, <idx> ;
> yields <n x <ty>>
> but this example:
> %result = insertvalue {i32, float} %agg, 1, 0
> should the example be:
> %result = insertvalue {i32, float} %agg, i32 1, 0

Oops, fixed.

>> @@ -1518,9 +1519,11 @@
>> /// element value from an aggregate value.
>> ///
>> class ExtractValueInst : public Instruction {
>> +  SmallVector<unsigned, 4> Indices;
>
> The indices of an instruction are fixed when the instruction is
> created.  Would it be reasonable to just make this be a new
> unsigned[]'d array instead of a smallvector?

SmallVector is a convenient way to co-allocate the indices with
the instruction in the common cases. We can certainly look at
doing custom allocation strategies to allocate the indices after
the end of the ExtractValueInst though.

>
>> @@ -1563,9 +1567,9 @@
>>
>>     if (NumIdx > 0)
>>       // This requires that the iterator points to contiguous memory.
>> -      return getIndexedType(Ptr, (Value *const *)&*IdxBegin, NumIdx);
>> +      return getIndexedType(Ptr, (const unsigned *)&*IdxBegin,
>> NumIdx);
>
> Is this cast needed?  It seems that it could paper over serious bugs.

Nope, fixed.

>
>> @@ -1575,55 +1579,53 @@
>>   /// Constructors - These two constructors are convenience methods
>> because one
>>   /// and two index extractvalue instructions are so common.
>> -  ExtractValueInst(Value *Agg, Value *Idx, const std::string &Name
>> = "",
>> +  ExtractValueInst(Value *Agg, unsigned Idx, const std::string
>> &Name = "",
>>                     Instruction *InsertBefore = 0);
>> -  ExtractValueInst(Value *Agg, Value *Idx,
>> +  ExtractValueInst(Value *Agg, unsigned Idx,
>>                     const std::string &Name, BasicBlock *InsertAtEnd);
>
> Since the ctors are private, who are they a convenience for?  It seems
> that the ::Create methods are the things that should exist, but extra
> constructors aren't need.

fixed.

>
>> public:
>> +  // allocate space for exactly two operands
>> +  void *operator new(size_t s) {
>> +    return User::operator new(s, 1);
>> +  }
>
> Plz update comment.

That was fixed already :-).

>> +
>> +const SmallVector<unsigned, 4> &ConstantExpr::getIndices() const {
>> +  if (const ExtractValueConstantExpr *EVCE =
>> +        dyn_cast<ExtractValueConstantExpr>(this))
>> +    return EVCE->Indices;
>> +  if (const InsertValueConstantExpr *IVCE =
>> +        dyn_cast<InsertValueConstantExpr>(this))
>> +    return IVCE->Indices;
>> +  assert(0 && "ConstantExpr does not have indices!");
>> +}
>
> Eliminating them also allows these methods to go away.  If you choose
> to keep them, please change getindices to be:
>
> ..
>> +   return cast<InsertValueConstantExpr>(this)->Indices;
>
>
> eliminating the assert(0)

fixed.

>>
>> +    assert(OpNo == 0 && "ExtractaValue has only one operand!");
>> +    const SmallVector<unsigned, 4> Indices = getIndices();
>
> No need to copy the vector here.

fixed.

>>
>> +    return
>> +      ConstantExpr::getExtractValue(Op, &Indices[0], Indices.size());
>>   }
>> +  case Instruction::InsertValue: {
>> +    const SmallVector<unsigned, 4> Indices = getIndices();
>
> likewise.

fixed.

>
>>
>> +    return ConstantExpr::getInsertValue(Ops[0], Ops[1],
>> +                                        &Indices[0], Indices.size());
>> +  }
>> +  case Instruction::ExtractValue: {
>> +    const SmallVector<unsigned, 4> Indices = getIndices();
>
> likewise.

fixed.

Dan