[llvm-commits] [PATCH] extend analysis passes to treat malloc calls the same as MallocInst (patch #2)
Victor Hernandez
vhernandez at apple.com
Fri Sep 18 08:55:46 PDT 2009
Thanks for the feedback, Eli.
Eli Friedman wrote:
> On Tue, Sep 15, 2009 at 1:44 PM, Victor Hernandez <vhernandez at apple.com
> > wrote:
>> These are the changes to make all the analysis passes apply the same
>> analysis to malloc calls as to MallocInst.
>
> Index: lib/Analysis/AliasAnalysis.cpp
> ===================================================================
> --- lib/Analysis/AliasAnalysis.cpp (revision 81898)
> +++ lib/Analysis/AliasAnalysis.cpp (working copy)
> @@ -31,6 +31,7 @@
> #include "llvm/IntrinsicInst.h"
> #include "llvm/Instructions.h"
> #include "llvm/Type.h"
> +#include "llvm/Analysis/MallocHelper.h"
> #include "llvm/Target/TargetData.h"
> using namespace llvm;
>
> @@ -239,7 +240,7 @@
> /// NoAlias returns
> ///
> bool llvm::isIdentifiedObject(const Value *V) {
> - if (isa<AllocationInst>(V) || isNoAliasCall(V))
> + if (isa<AllocationInst>(V) || isMalloc(V) || isNoAliasCall(V))
> return true;
> if (isa<GlobalValue>(V) && !isa<GlobalAlias>(V))
> return true;
>
> Unnecessary change, assuming malloc gets a noalias attribute;
> isIdentifiedObject is only used on values with the bitcasts stripped
> off.
-simplify-libcalls gives malloc the noalias attribute. So I'll remove
this change and update any tests that rely on malloc being noalias to
run -simplify-libcalls as well.
>
> Index: lib/Analysis/BasicAliasAnalysis.cpp
> ===================================================================
> --- lib/Analysis/BasicAliasAnalysis.cpp (revision 81898)
> +++ lib/Analysis/BasicAliasAnalysis.cpp (working copy)
> @@ -15,6 +15,7 @@
>
> #include "llvm/Analysis/AliasAnalysis.h"
> #include "llvm/Analysis/CaptureTracking.h"
> +#include "llvm/Analysis/MallocHelper.h"
> #include "llvm/Analysis/Passes.h"
> #include "llvm/Constants.h"
> #include "llvm/DerivedTypes.h"
> @@ -82,7 +83,7 @@
> /// object that never escapes from the function.
> static bool isNonEscapingLocalObject(const Value *V) {
> // If this is a local allocation, check to see if it escapes.
> - if (isa<AllocationInst>(V) || isNoAliasCall(V))
> + if (isa<AllocationInst>(V) || isMalloc(V) || isNoAliasCall(V))
> return !PointerMayBeCaptured(V, false);
>
> // If this is an argument that corresponds to a byval or noalias
> argument,
>
> Same as above.
Removed like above.
>
> @@ -111,6 +112,20 @@
> AccessTy = AI->getType()->getElementType();
> else
> return false;
> + } else if (const CallInst* CI = extractMallocCall(V)) {
> + if (isArrayMalloc(V))
> + return false;
> + if (const Type* PT = getMallocAllocatedType(CI))
> + AccessTy = PT;
> + else
> + return false;
> + } else if (const CallInst* CI = extractMallocCallFromBitCast(V)) {
> + if (isArrayMalloc(V))
> + return false;
> + if (const Type* PT = getMallocAllocatedType(CI))
> + AccessTy = PT;
> + else
> + return false;
> } else if (const Argument *A = dyn_cast<Argument>(V)) {
> if (A->hasByValAttr())
> AccessTy = cast<PointerType>(A->getType())->getElementType();
>
> It would be much more straightforward to just use the size argument to
> the malloc here; also, V is never a bitcast.
How is this?
@@ -111,6 +112,12 @@
AccessTy = AI->getType()->getElementType();
else
return false;
+ } else if (const CallInst* CI = extractMallocCall(V)) {
+ if (!isArrayMalloc(V))
+ // The size is the argument to the malloc call.
+ if (const ConstantInt* C = dyn_cast<ConstantInt>(CI->getOperand
(1)))
+ return (C->getZExtValue() < Size);
+ return false;
} else if (const Argument *A = dyn_cast<Argument>(V)) {
if (A->hasByValAttr())
AccessTy = cast<PointerType>(A->getType())->getElementType();
>
> @@ -328,8 +343,8 @@
> return NoAlias;
>
> // Arguments can't alias with local allocations or noalias calls.
> - if ((isa<Argument>(O1) && (isa<AllocationInst>(O2) ||
> isNoAliasCall(O2))) ||
> - (isa<Argument>(O2) && (isa<AllocationInst>(O1) ||
> isNoAliasCall(O1))))
> + if ((isa<Argument>(O1) && (isa<AllocationInst>(O2) ||
> isMalloc(O2) || isNoAliasCall(O2))) ||
> + (isa<Argument>(O2) && (isa<AllocationInst>(O1) ||
> isMalloc(O1) || isNoAliasCall(O1))))
> return NoAlias;
>
> // Most objects can't alias null.
>
> Unnecessary change, assuming malloc gets a noalias attribute.
Fixed as above.
>
> Index: lib/Analysis/PointerTracking.cpp
> ===================================================================
> --- lib/Analysis/PointerTracking.cpp (revision 81898)
> +++ lib/Analysis/PointerTracking.cpp (working copy)
> @@ -13,6 +13,7 @@
> #include "llvm/Analysis/ConstantFolding.h"
> #include "llvm/Analysis/Dominators.h"
> #include "llvm/Analysis/LoopInfo.h"
> +#include "llvm/Analysis/MallocHelper.h"
> #include "llvm/Analysis/PointerTracking.h"
> #include "llvm/Analysis/ScalarEvolution.h"
> #include "llvm/Analysis/ScalarEvolutionExpressions.h"
> @@ -99,6 +100,22 @@
> return SE->getSCEV(arraySize);
> }
>
> + if (CallInst *CI = extractMallocCall(V)) {
> + Value *arraySize = getMallocArraySize(CI);
> + Ty = getMallocAllocatedType(CI);
> + if (!Ty || !arraySize) return SE->getCouldNotCompute();
> + // arraySize elements of type Ty.
> + return SE->getSCEV(arraySize);
> + }
> +
> + if (CallInst *CI = extractMallocCallFromBitCast(V)) {
> + Value *arraySize = getMallocArraySize(CI);
> + Ty = getMallocAllocatedType(CI);
> + if (!Ty || !arraySize) return SE->getCouldNotCompute();
> + // arraySize elements of type Ty.
> + return SE->getSCEV(arraySize);
> + }
> +
> if (GlobalVariable *GV = dyn_cast<GlobalVariable>(V)) {
> if (GV->hasDefinitiveInitializer()) {
> Constant *C = GV->getInitializer();
>
> Will this mess up with malloc used as a char array?
You are right. mallocs of i8 arrays were not being detected
correctly, because they don't have a bitcast usage. I have updated
getMallocType() and isArrayMallocHelper() to fix this:
-static bool isArrayMallocHelper(const CallInst *CI) {
+static bool isArrayMallocHelper(const CallInst *CI, LLVMContext
&Context,
+ const TargetData* TD) {
if (!CI)
return false;
- // Only identify array mallocs for mallocs with 1 bitcast use. The
unique
- // bitcast is needed to determine the type/size of the array
allocation.
- if (!CI->hasOneUse()) return false;
+ const Type* T = getMallocAllocatedType(CI);
- for (Value::use_const_iterator UI = CI->use_begin(), E = CI->use_end
();
- UI != E; )
- if (!isa<BitCastInst>(cast<Instruction>(*UI++)))
- return false;
+ // We can only indentify an array malloc if we know the type of the
malloc
+ // call.
+ if (!T) return false;
- // malloc arg
Value* MallocArg = CI->getOperand(1);
- // element size
- const Type* T = getMallocAllocatedType(CI);
- if (!T) return false;
Constant *ElementSize = ConstantExpr::getSizeOf(T);
-
+ ElementSize = ConstantExpr::getTruncOrBitCast(ElementSize,
+ MallocArg->getType());
+ Constant *FoldedElementSize = ConstantFoldConstantExpression(
+ cast<ConstantExpr>(ElementSize),
+ Context, TD);
+
+
if (isa<ConstantExpr>(MallocArg))
- return (MallocArg == ElementSize) ? false : true;
+ return (MallocArg != ElementSize);
BinaryOperator *BI = dyn_cast<BinaryOperator>(MallocArg);
if (!BI)
return false;
- if (BI->getOpcode() != Instruction::Mul)
- return false;
+ if (BI->getOpcode() == Instruction::Mul) {
+ // ArraySize * ElementSize
+ if (BI->getOperand(1) == ElementSize ||
+ (FoldedElementSize && BI->getOperand(1) == FoldedElementSize))
- if (BI->getOperand(1) != ElementSize)
+ return true;
+
return false;
-
- return true;
+ }
+
+ return false;
}
/// getMallocType - Returns the PointerType resulting from the
malloc call.
@@ -142,14 +163,24 @@
assert(isMalloc(CI) && "GetMallocType and not malloc call");
const BitCastInst* BCI = NULL;
+
+ // Determine if CallInst has a bitcast use.
+ for (Value::use_const_iterator UI = CI->use_begin(), E = CI->use_end
();
+ UI != E; )
+ if ((BCI = dyn_cast<BitCastInst>(cast<Instruction>(*UI++))))
+ break;
- // Determine type only if there is only 1 bitcast use of CI.
- if (CI->hasOneUse())
- for (Value::use_const_iterator UI = CI->use_begin(), E = CI-
>use_end();
- UI != E; )
- BCI = dyn_cast<BitCastInst>(cast<Instruction>(*UI++));
+ // Malloc call has 1 bitcast use and no other uses, so type is the
bitcast's
+ // destination type.
+ if (BCI && CI->hasOneUse())
+ return cast<PointerType>(BCI->getDestTy());
- return BCI ? reinterpret_cast<const PointerType*>(BCI->getDestTy
()) : NULL;
+ // Malloc call was not bitcast, so the type is the malloc's return
type, i8*.
+ if (!BCI)
+ return cast<PointerType>(CI->getType());
+
+ // Type could not be determined.
+ return NULL;
}
Also, I got rid of the unnecessary extractMallocCallFromBitCast if
case while I was looking at this code.
>
> Index: lib/Analysis/ValueTracking.cpp
> ===================================================================
> --- lib/Analysis/ValueTracking.cpp (revision 81898)
> +++ lib/Analysis/ValueTracking.cpp (working copy)
> @@ -20,6 +20,7 @@
> #include "llvm/IntrinsicInst.h"
> #include "llvm/LLVMContext.h"
> #include "llvm/Operator.h"
> +#include "llvm/Analysis/MallocHelper.h"
> #include "llvm/Target/TargetData.h"
> #include "llvm/Support/GetElementPtrTypeIterator.h"
> #include "llvm/Support/MathExtras.h"
> @@ -621,6 +622,24 @@
> break;
> }1
> }
> + } else if (CallInst* CI = extractMallocCall(I)) {
> + unsigned Align = 0;
> + const Type* T = getMallocAllocatedType(CI);
> + if (TD && T) {
> + // Malloc returns maximally aligned memory.
> + Align = TD->getABITypeAlignment(T);
> + Align =
> + std::max(Align,
> + (unsigned)TD->getABITypeAlignment(
> + Type::getDoubleTy(V->getContext())));
> + Align =
> + std::max(Align,
> + (unsigned)TD->getABITypeAlignment(
> + Type::getInt64Ty(V->getContext())));
> + }
> + if (Align > 0)
> + KnownZero = Mask & APInt::getLowBitsSet(BitWidth,
> +
> CountTrailingZeros_32(Align));
> }
> break;
> }
>
> This is simply wrong; we can't make that guarantee for malloc.
> Consider, for example, a malloc used as an SSE vector on Windows.
I agree that this is wrong; yet the bug you describe currently exists
in LLVM TOT and results in better codegen on some systems.
RaiseAllocations creates a MallocInst with no specified alignment.
KnownZero for MallocInst ends up getting the wrong alignment, just
like malloc calls would with this code.
I am not sure what to do in this scenario. I agree that getting rid
of this code is correct for the example you mention, but the code is
currently resulting in aligned instructions being generated on MacOSX/
x86, so it's tempting to keep it in.
> More generally, getMallocAllocatedType seems a bit suspect in that an
> i8 array will get messed up by the change to getMallocAllocatedType.
See above.
>
> -Eli
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20090918/4d6d64af/attachment.html>
More information about the llvm-commits
mailing list