[llvm-commits] [llvm] r48254 - in /llvm/trunk: lib/Transforms/IPO/IPConstantPropagation.cpp test/Transforms/IPConstantProp/return-constants.ll

[Chris Lattner]

On Mar 11, 2008, at 3:24 PM, Devang Patel wrote:
> URL: http://llvm.org/viewvc/llvm-project?rev=48254&view=rev
> Log:
> Handle multiple ret values.

>
> +++ llvm/trunk/lib/Transforms/IPO/IPConstantPropagation.cpp Tue Mar  
> 11 17:24:29 2008
> @@ -142,22 +143,44 @@
>     return false; // No return value.
>
>   // Check to see if this function returns a constant.
> -  Value *RetVal = 0;
> +  SmallVector<Value *,4> RetVals;
> +  unsigned N = 0;
> +  const StructType *STy = dyn_cast<StructType>(F.getReturnType());
> +  if (STy)
> +    N = STy->getNumElements();
> +  else
> +    N = 1;
> +  for (unsigned i = 0; i < N; ++i)
> +    RetVals.push_back(0);

Instead of introducing N, I'd suggest just using:

> +  if (STy)
> +    RetVals.assign(STy->getNumElements(), 0);
> +  else
> +    RetVals.push_back(0);

then using RetVals.size() instead of N.

>   for (Function::iterator BB = F.begin(), E = F.end(); BB != E; ++BB)
>     if (ReturnInst *RI = dyn_cast<ReturnInst>(BB->getTerminator())) {
> +      assert (N == RI->getNumOperands() && "Invalid ReturnInst  
> operands!");
> +      for (unsigned i = 0; i < N; ++i) {
> +        if (isa<UndefValue>(RI->getOperand(i))) {
> +          // Ignore
> +        } else if (Constant *C = dyn_cast<Constant>(RI- 
> >getOperand(i))) {
> +          Value *RV = RetVals[i];
> +          if (RV == 0)
> +            RetVals[i] = C;
> +          else if (RV != C)
> +            return false; // Does not return the same constant.

This isn't right: some retvals could be constant while others aren't.   
You need a value to represent "not constant".  If this happens, set  
the value to 'not constant' and count how many values get marked 'not  
constant'.  If that count ever equals RetVals.size(), *then* you  
return false.

>
> +        } else {
> +          return false; // Does not return a constant.

Likewise.

> +  if (N == 1) {
> +    if (RetVals[0] == 0)
> +      RetVals[0] = UndefValue::get(F.getReturnType());
> +  } else {
> +    for (unsigned i = 0; i < N; ++i) {
> +      Value *RetVal = RetVals[i];
> +      if (RetVal == 0)
> +        RetVals[i] = UndefValue::get(STy->getElementType(i));
> +    }
> +  }

Why is the N=1 case special here?  Also, the RetVal temporary seems  
unneeded.

> @@ -172,8 +195,18 @@
>           CS.getCalledFunction() != &F) {
>         ReplacedAllUsers = false;
>       } else {
> +        Instruction *Call = CS.getInstruction();
> +        if (!Call->use_empty()) {
> +          if (N == 1)
> +            Call->replaceAllUsesWith(RetVals[0]);
> +          else {
> +            for(Value::use_iterator CUI = Call->use_begin(), CUE =  
> Call->use_end();
> +                CUI != CUE; ++CUI) {
> +              GetResultInst *GR = cast<GetResultInst>(CUI);
> +              GR->replaceAllUsesWith(RetVals[GR->getIndex()]);
> +              GR->eraseFromParent();

This can crash, because there may be some values that are still null.

>
> +            }
> +          }
>           MadeChange = true;
>         }
>       }

IPCP should handle invoke too.

> @@ -182,15 +215,20 @@
>   // If we replace all users with the returned constant, and there  
> can be no
>   // other callers of the function, replace the constant being  
> returned in the
>   // function with an undef value.
> +  if (ReplacedAllUsers && F.hasInternalLinkage()) {
> +    for (unsigned i = 0; i < N; ++i) {
> +      Value *RetVal = RetVals[i];
> +      if (isa<UndefValue>(RetVal))
> +        continue;
> +      Value *RV = UndefValue::get(RetVal->getType());
> +      for (Function::iterator BB = F.begin(), E = F.end(); BB != E;  
> ++BB)
> +        if (ReturnInst *RI = dyn_cast<ReturnInst>(BB- 
> >getTerminator())) {
> +          if (RI->getOperand(i) != RV) {
> +            RI->setOperand(i, RV);
> +            MadeChange = true;
> +          }
>         }

This loop is nested wrong.  You should loop over the whole function  
once, and for each return, you should loop over the ret vals.  This is  
much more efficient.

-Chris