[llvm-commits] [llvm] r47348 - in /llvm/trunk: include/llvm/Instruction.def include/llvm/Instructions.h include/llvm/Support/InstVisitor.h lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp lib/VMCore/Instructions.cpp lib/VMCore/Verifier.cpp

[Chris Lattner]

On Feb 19, 2008, at 2:15 PM, Devang Patel wrote:
> URL: http://llvm.org/viewvc/llvm-project?rev=47348&view=rev
> Log:
> Add GetResultInst. First step for multiple return value support.

Woot!  Please update LangRef.html though :)

> +// 
> = 
> = 
> = 
> ----------------------------------------------------------------------= 
> ==//
> +//                             GetResultInst Class
> +// 
> = 
> = 
> = 
> ----------------------------------------------------------------------= 
> ==//
> +
> +/// GetResultInst - This instruction extracts individual result  
> value from
> +/// aggregate value, where aggregate value is returned by CallInst.
> +///

>
> +  explicit GetResultInst(Value *Aggr, Value *Index,

I think the Index should be specified as an unsigned, not a Value*.   
Supporting an arbitrary Value* means that we'd have to support codegen  
of getresult with a variable index.  Since this isn't very useful to  
us, I suggest that we just make getresult take a single Value* and an  
unsigned constant that is the value to extract from.

> +  // getType - Get aggregate value element type
> +  inline const Type *getType() const {
> +    return Ops[0]->getType();
> +  }

I don't think this is right: getType() on the getresult should return  
the scalar result type, not the input operand type.

>
> +  Value *getAggregateValue() {
> +    return getOperand(0);
> +  }

Ok, it would also be nice to have a const version that returns a const  
Value*.

> +  const Value *geIndex() {
> +    return getOperand(1);
> +  }

geIndex -> getIndex(), should also return an unsigned, as above.

> Modified: llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp?rev=47348&r1=47347&r2=47348&view=diff
>
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> ======================================================================
> --- llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp  
> (original)
> +++ llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp Tue Feb  
> 19 16:15:16 2008
> @@ -608,6 +608,10 @@
>
>   void visitMemIntrinsic(CallInst &I, unsigned Op);
>
> +  void visitGetResult(GetResultInst &I) {
> +    // FIXME

Please make this: assert(0 && "getresult unimplemented");

> +// 
> = 
> = 
> = 
> ----------------------------------------------------------------------= 
> ==//
> +//                           GetResultInst Implementation
> +// 
> = 
> = 
> = 
> ----------------------------------------------------------------------= 
> ==//
> +
> +GetResultInst::GetResultInst(Value *Aggr, Value *Index,
> +                             const std::string &Name,
> +                             Instruction *InsertBef)
> +  : Instruction(Aggr->getType(),
> +                GetResult, Ops, 2, InsertBef) {

The type of the instruction should be the type of the "Index" field in  
the Aggr struct, not the struct itself.  If you have:

%x = getresult { int, float} %y, 1

the type of %x should be float, not {int,float}.

> +void Verifier::visitGetResultInst(GetResultInst &GRI) {
> +  // FIXME : Check operands.
> +  visitInstruction(GRI);
> +}

I'd suggest checking isValidOperands here, and asserting that the  
result type of the getresult is the same as the respective field type  
of struct.

-Chris