[llvm-commits] [llvm] r47508 - in /llvm/trunk: include/llvm/Instructions.h include/llvm/User.h lib/AsmParser/llvmAsmParser.y lib/VMCore/AsmWriter.cpp lib/VMCore/Instructions.cpp lib/VMCore/Verifier.cpp

[Chris Lattner]

> URL: http://llvm.org/viewvc/llvm-project?rev=47508&view=rev
> Log:
> To support multiple return values, now ret instruction supports  
> multiple operands instead of one aggregate operand.

Some more details:

> = 
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> ======================================================================
> --- llvm/trunk/include/llvm/Instructions.h (original)
> +++ llvm/trunk/include/llvm/Instructions.h Fri Feb 22 18:35:18 2008
> @@ -1379,9 +1379,9 @@
> /// does not continue in this function any longer.
> ///
> class ReturnInst : public TerminatorInst {
> -  Use RetVal;  // Return Value: null if 'void'.
>   ReturnInst(const ReturnInst &RI);
>   void init(Value *RetVal);
> +  void init(std::vector<Value *> &RetVals);

I'd suggest deleting the first version of 'init' here, and just use:

   void init(Value * const *RetVals, unsigned NumRetVals);

which allows you to efficiently handle both with one function.

> @@ -1397,21 +1397,15 @@
>   // if it was passed NULL.
>   explicit ReturnInst(Value *retVal = 0, Instruction *InsertBefore =  
> 0);
>   ReturnInst(Value *retVal, BasicBlock *InsertAtEnd);
> +  ReturnInst(std::vector<Value *> &retVals);
> +  ReturnInst(std::vector<Value *> &retVals, Instruction  
> *InsertBefore);
> +  ReturnInst(std::vector<Value *> &retVals, BasicBlock *InsertAtEnd);

These should all take const vectors by reference.  It would also be  
useful to provide versions that take pointer to pointer + num results  
to allow clients to use arrays and smallvectors or some other linear  
container.


> Modified: llvm/trunk/lib/VMCore/Instructions.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/VMCore/Instructions.cpp?rev=47508&r1=47507&r2=47508&view=diff
>
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> ======================================================================
> --- llvm/trunk/lib/VMCore/Instructions.cpp (original)
> +++ llvm/trunk/lib/VMCore/Instructions.cpp Fri Feb 22 18:35:18 2008
> @@ -573,34 +573,75 @@
>
> ReturnInst::ReturnInst(Value *retVal, Instruction *InsertBefore)
> -  : TerminatorInst(Type::VoidTy, Instruction::Ret, &RetVal, 0,  
> InsertBefore) {
> +  : TerminatorInst(Type::VoidTy, Instruction::Ret, OperandList, 0,  
> InsertBefore) {

This should pass up null instead of OperandList.  At this point,  
OperandList is uninitialized and you're initializing it with itself.   
Likewise for the other ctors.

> void ReturnInst::init(Value *retVal) {
>   if (retVal && retVal->getType() != Type::VoidTy) {

This is very strange.  I know it's not your fault, but could you check  
to see whether you can change this to:

    if (retVal) {
      assert(retVal->getType() != VoidTy);

It doesn't make sense to support passing in a void value to create a  
void return.

>     assert(!isa<BasicBlock>(retVal) &&
>            "Cannot return basic block.  Probably using the incorrect  
> ctor");
>     NumOperands = 1;
> -    RetVal.init(retVal, this);
> +    Use *OL = OperandList = new Use[1];
> +    OL[0].init(retVal, this);

As an important optimization for the common case, please put a single  
'Use' in the return instruction.  It should only be used it if there  
is one return value: in the case when there are multiple retvals, just  
ignore it.  The old version of ReturnInst did this optimization: in  
the case of 'ret void' the operand went unused.

> +void ReturnInst::init(std::vector<Value *> &retVals) {

As above, please merge the two 'init' methods.

> +Value *ReturnInst::getReturnValue(unsigned n) const {

This should assert that n < NumOperands, and drop the return of null.   
If existing clients depend on the return of null behavior, they will  
need to be updated.  Once you do this, this should be moved inline.

Also, const methods should return const Value*'s.  You'll need to  
duplicate this into two methods.  One that is const and returns const,  
one that is not const and returns nonconst.

> +ReturnInst::~ReturnInst() {
> +  if (NumOperands)
> +    delete [] OperandList;
> +}

This will need to be:
   if (NumOperands != 1) delete [] OperandList;

> = 
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> ======================================================================
> --- llvm/trunk/lib/VMCore/Verifier.cpp (original)
> +++ llvm/trunk/lib/VMCore/Verifier.cpp Fri Feb 22 18:35:18 2008
> @@ -576,14 +576,22 @@
>
> void Verifier::visitReturnInst(ReturnInst &RI) {
>   Function *F = RI.getParent()->getParent();
> -  if (RI.getNumOperands() == 0)
> +  unsigned N = RI.getNumOperands();
> +  if (N == 0)
>     Assert2(F->getReturnType() == Type::VoidTy,
>             "Found return instr that returns void in Function of non- 
> void "
>             "return type!", &RI, F->getReturnType());
> -  else
> +  else if (N == 1)
>     Assert2(F->getReturnType() == RI.getOperand(0)->getType(),
>             "Function return type does not match operand "
>             "type of return inst!", &RI, F->getReturnType());
> +  else {
> +    const StructType *STy = cast<StructType>(F->getReturnType());

This cast assumes that if the return has multiple operands that the  
function returns multiple results.  The verifier should check  
assumptions like this.

>
> +    for (unsigned i = 0; i < N; i++)

You should also check that the number of elements in the struct match  
up with # return operands.

-Chris