[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

[Chris Lattner]

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.

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

> +++ llvm/trunk/include/llvm/Constants.h Fri May 30 19:58:22 2008
> public:
>   // Static methods to construct a ConstantExpr of different kinds.   
> Note that
> @@ -656,6 +657,10 @@
>   /// @brief Return true if this is a compare constant expression
>   bool isCompare() const;
>
> +  /// @brief Return true if this is an insertvalue or extractvalue  
> expression,
> +  /// and the getIndices() method may be used.
> +  bool hasIndices() const;
> +
> +  /// getIndices - Assert that this is an insertvalue or exactvalue
> +  /// expression and return the list of indices.
> +  const SmallVector<unsigned, 4> &getIndices() const;

Is there ever a case where insert/extract value can't be constant  
folded?

> = 
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> ======================================================================
> --- llvm/trunk/include/llvm/Instructions.h (original)
> +++ llvm/trunk/include/llvm/Instructions.h Fri May 30 19:58:22 2008
> @@ -22,6 +22,7 @@
> #include "llvm/DerivedTypes.h"
> #include "llvm/ParameterAttributes.h"
> #include "llvm/BasicBlock.h"
> +#include "llvm/ADT/SmallVector.h"
>
> namespace llvm {
>
> @@ -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?

> @@ -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.

> @@ -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.

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

Plz update comment.

> @@ -1733,9 +1729,12 @@
> /// value into an aggregate value.
> ///
> class InsertValueInst : public Instruction {
> +  SmallVector<unsigned, 4> Indices;

The above all applies to InsertValueInst as well.

> +++ llvm/trunk/lib/VMCore/Constants.cpp Fri May 30 19:58:22 2008
> @@ -537,15 +537,23 @@
> /// Constants.cpp, and is used behind the scenes to implement
> /// extractvalue constant exprs.
> class VISIBILITY_HIDDEN ExtractValueConstantExpr : public  
> ConstantExpr {

Can't Extract/InsertValue always be constant folded? If so, there is  
no reason to be able to represent them.

> @@ -718,6 +703,21 @@
>   return getOpcode() == Instruction::ICmp || getOpcode() ==  
> Instruction::FCmp;
> }
>
> +bool ConstantExpr::hasIndices() const {
> +  return getOpcode() == Instruction::ExtractValue ||
> +         getOpcode() == Instruction::InsertValue;
> +}
> +
> +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)
>
> +    assert(OpNo == 0 && "ExtractaValue has only one operand!");
> +    const SmallVector<unsigned, 4> Indices = getIndices();

No need to copy the vector here.
>
> +    return
> +      ConstantExpr::getExtractValue(Op, &Indices[0], Indices.size());
>   }
> +  case Instruction::InsertValue: {
> +    const SmallVector<unsigned, 4> Indices = getIndices();

likewise.

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

likewise.

-Chris
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20080622/bfc04bce/attachment.html>