[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

[Devang Patel]

On Feb 19, 2008, at 8:16 PM, Chris Lattner wrote:

> 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 :)

sure. I'll update docs once things settle down.

>> +//
>> =
>> =
>> =
>> ----------------------------------------------------------------------=
>> ==//
>> +//                             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.

okay, done.

>
>
>> +  // 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.

yup. In fact I do not see any reason to override getType() here.
>
>
>>
>> +  Value *getAggregateValue() {
>> +    return getOperand(0);
>> +  }
>
> Ok, it would also be nice to have a const version that returns a const
> Value*.

done

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

oops. fixed.

> should also return an unsigned, as above.

ok

>
>
>> 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");

sure

>
>
>> +//
>> =
>> =
>> =
>> ----------------------------------------------------------------------=
>> ==//
>> +//                           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}.

done

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

done.

Thanks for review!
-
Devang