[llvm-commits] [llvm] r149457 - in /llvm/trunk: lib/Transforms/InstCombine/InstCombineCalls.cpp test/Transforms/InstCombine/2008-01-06-BitCastAttributes.ll test/Transforms/InstCombine/call.ll

[Eli Friedman]

On Tue, Jan 31, 2012 at 4:08 PM, Jim Grosbach <grosbach at apple.com> wrote:
> Author: grosbach
> Date: Tue Jan 31 18:08:17 2012
> New Revision: 149457
>
> URL: http://llvm.org/viewvc/llvm-project?rev=149457&view=rev
> Log:
> Disable InstCombine unsafe folding bitcasts of calls w/ varargs.
>
> Changing arguments from being passed as fixed to varargs is unsafe, as
> the ABI may require they be handled differently (stack vs. register, for
> example).
>
> Remove two tests which rely on the bitcast being folded into the direct
> call, which is exactly the transformation that's unsafe.
>
> Removed:
>    llvm/trunk/test/Transforms/InstCombine/2008-01-06-BitCastAttributes.ll
> Modified:
>    llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp
>    llvm/trunk/test/Transforms/InstCombine/call.ll
>
> Modified: llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp?rev=149457&r1=149456&r2=149457&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp (original)
> +++ llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp Tue Jan 31 18:08:17 2012
> @@ -1105,21 +1105,12 @@
>     if (FT->isVarArg()!=cast<FunctionType>(APTy->getElementType())->isVarArg())
>       return false;
>   }
> -
> -  if (FT->getNumParams() < NumActualArgs && FT->isVarArg() &&
> -      !CallerPAL.isEmpty())
> -    // In this case we have more arguments than the new function type, but we
> -    // won't be dropping them.  Check that these extra arguments have attributes
> -    // that are compatible with being a vararg call argument.
> -    for (unsigned i = CallerPAL.getNumSlots(); i; --i) {
> -      if (CallerPAL.getSlot(i - 1).Index <= FT->getNumParams())
> -        break;
> -      Attributes PAttrs = CallerPAL.getSlot(i - 1).Attrs;
> -      if (PAttrs & Attribute::VarArgsIncompatible)
> -        return false;
> -    }
>
> -
> +  // If we're casting varargs to non-varargs, that may not be allowable
> +  // under the ABI, so conservatively don't do anything.
> +  if (FT->getNumParams() < NumActualArgs && FT->isVarArg())
> +    return false;

This comment doesn't make sense given the code above this: if we can't
see the definition, we won't reach this check, and if we can see the
definition we're casting we're casting varargs to non-varargs, we
can't possibly be introducing undefined behavior.

-Eli