<html><head></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><div><div>Thanks for the feedback, Eli.</div><div><br></div><div>Eli Friedman wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div>On Tue, Sep 15, 2009 at 1:44 PM, Victor Hernandez <<a href="mailto:vhernandez@apple.com">vhernandez@apple.com</a>> wrote:<br><blockquote type="cite">These are the changes to make all the analysis passes apply the same<br></blockquote><blockquote type="cite">analysis to malloc calls as to MallocInst.<br></blockquote><br>Index: lib/Analysis/AliasAnalysis.cpp<br>===================================================================<br>--- lib/Analysis/AliasAnalysis.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 81898)<br>+++ lib/Analysis/AliasAnalysis.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(working copy)<br>@@ -31,6 +31,7 @@<br> #include "llvm/IntrinsicInst.h"<br> #include "llvm/Instructions.h"<br> #include "llvm/Type.h"<br>+#include "llvm/Analysis/MallocHelper.h"<br> #include "llvm/Target/TargetData.h"<br> using namespace llvm;<br><br>@@ -239,7 +240,7 @@<br> /// NoAlias returns<br> ///<br> bool llvm::isIdentifiedObject(const Value *V) {<br>- if (isa<AllocationInst>(V) || isNoAliasCall(V))<br>+ if (isa<AllocationInst>(V) || isMalloc(V) || isNoAliasCall(V))<br> return true;<br> if (isa<GlobalValue>(V) && !isa<GlobalAlias>(V))<br> return true;<br><br>Unnecessary change, assuming malloc gets a noalias attribute;<br>isIdentifiedObject is only used on values with the bitcasts stripped<br>off.<br></div></blockquote><div><br></div>-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.<br><blockquote type="cite"><div><br>Index: lib/Analysis/BasicAliasAnalysis.cpp<br>===================================================================<br>--- lib/Analysis/BasicAliasAnalysis.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 81898)<br>+++ lib/Analysis/BasicAliasAnalysis.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(working copy)<br>@@ -15,6 +15,7 @@<br><br> #include "llvm/Analysis/AliasAnalysis.h"<br> #include "llvm/Analysis/CaptureTracking.h"<br>+#include "llvm/Analysis/MallocHelper.h"<br> #include "llvm/Analysis/Passes.h"<br> #include "llvm/Constants.h"<br> #include "llvm/DerivedTypes.h"<br>@@ -82,7 +83,7 @@<br> /// object that never escapes from the function.<br> static bool isNonEscapingLocalObject(const Value *V) {<br> // If this is a local allocation, check to see if it escapes.<br>- if (isa<AllocationInst>(V) || isNoAliasCall(V))<br>+ if (isa<AllocationInst>(V) || isMalloc(V) || isNoAliasCall(V))<br> return !PointerMayBeCaptured(V, false);<br><br> // If this is an argument that corresponds to a byval or noalias argument,<br><br>Same as above.<br></div></blockquote><div><br></div>Removed like above.</div><div><br><blockquote type="cite"><div><br>@@ -111,6 +112,20 @@<br> AccessTy = AI->getType()->getElementType();<br> else<br> return false;<br>+ } else if (const CallInst* CI = extractMallocCall(V)) {<br>+ if (isArrayMalloc(V))<br>+ return false;<br>+ if (const Type* PT = getMallocAllocatedType(CI))<br>+ AccessTy = PT;<br>+ else<br>+ return false;<br>+ } else if (const CallInst* CI = extractMallocCallFromBitCast(V)) {<br>+ if (isArrayMalloc(V))<br>+ return false;<br>+ if (const Type* PT = getMallocAllocatedType(CI))<br>+ AccessTy = PT;<br>+ else<br>+ return false;<br> } else if (const Argument *A = dyn_cast<Argument>(V)) {<br> if (A->hasByValAttr())<br> AccessTy = cast<PointerType>(A->getType())->getElementType();<br><br>It would be much more straightforward to just use the size argument to<br>the malloc here; also, V is never a bitcast.<br></div></blockquote><div><br></div>How is this?</div><div><div>@@ -111,6 +112,12 @@</div><div> AccessTy = AI->getType()->getElementType();</div><div> else</div><div> return false;</div><div>+ } else if (const CallInst* CI = extractMallocCall(V)) {</div><div>+ if (!isArrayMalloc(V))</div><div>+ // The size is the argument to the malloc call.</div><div>+ if (const ConstantInt* C = dyn_cast<ConstantInt>(CI->getOperand(1)))</div><div>+ return (C->getZExtValue() < Size);</div><div>+ return false;</div><div> } else if (const Argument *A = dyn_cast<Argument>(V)) {</div><div> if (A->hasByValAttr())</div><div> AccessTy = cast<PointerType>(A->getType())->getElementType();</div><div><br></div><blockquote type="cite"><div><br>@@ -328,8 +343,8 @@<br> return NoAlias;<br><br> // Arguments can't alias with local allocations or noalias calls.<br>- if ((isa<Argument>(O1) && (isa<AllocationInst>(O2) ||<br>isNoAliasCall(O2))) ||<br>- (isa<Argument>(O2) && (isa<AllocationInst>(O1) || isNoAliasCall(O1))))<br>+ if ((isa<Argument>(O1) && (isa<AllocationInst>(O2) ||<br>isMalloc(O2) || isNoAliasCall(O2))) ||<br>+ (isa<Argument>(O2) && (isa<AllocationInst>(O1) ||<br>isMalloc(O1) || isNoAliasCall(O1))))<br> return NoAlias;<br><br> // Most objects can't alias null.<br><br>Unnecessary change, assuming malloc gets a noalias attribute.<br></div></blockquote><div><br></div>Fixed as above.</div><div><br><blockquote type="cite"><div><br>Index: lib/Analysis/PointerTracking.cpp<br>===================================================================<br>--- lib/Analysis/PointerTracking.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 81898)<br>+++ lib/Analysis/PointerTracking.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(working copy)<br>@@ -13,6 +13,7 @@<br> #include "llvm/Analysis/ConstantFolding.h"<br> #include "llvm/Analysis/Dominators.h"<br> #include "llvm/Analysis/LoopInfo.h"<br>+#include "llvm/Analysis/MallocHelper.h"<br> #include "llvm/Analysis/PointerTracking.h"<br> #include "llvm/Analysis/ScalarEvolution.h"<br> #include "llvm/Analysis/ScalarEvolutionExpressions.h"<br>@@ -99,6 +100,22 @@<br> return SE->getSCEV(arraySize);<br> }<br><br>+ if (CallInst *CI = extractMallocCall(V)) {<br>+ Value *arraySize = getMallocArraySize(CI);<br>+ Ty = getMallocAllocatedType(CI);<br>+ if (!Ty || !arraySize) return SE->getCouldNotCompute();<br>+ // arraySize elements of type Ty.<br>+ return SE->getSCEV(arraySize);<br>+ }<br>+<br>+ if (CallInst *CI = extractMallocCallFromBitCast(V)) {<br>+ Value *arraySize = getMallocArraySize(CI);<br>+ Ty = getMallocAllocatedType(CI);<br>+ if (!Ty || !arraySize) return SE->getCouldNotCompute();<br>+ // arraySize elements of type Ty.<br>+ return SE->getSCEV(arraySize);<br>+ }<br>+<br> if (GlobalVariable *GV = dyn_cast<GlobalVariable>(V)) {<br> if (GV->hasDefinitiveInitializer()) {<br> Constant *C = GV->getInitializer();<br><br>Will this mess up with malloc used as a char array?<br></div></blockquote><div><br></div><div>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 <span class="Apple-style-span" style="font-family: Menlo; font-size: 11px; ">isArrayMallocHelper() <span class="Apple-style-span" style="font-family: Helvetica; font-size: medium; ">to fix this:</span></span></div><div><br></div><div><div>-static bool isArrayMallocHelper(const CallInst *CI) {</div><div>+static bool isArrayMallocHelper(const CallInst *CI, LLVMContext &Context,</div><div>+ const TargetData* TD) {</div><div> if (!CI)</div><div> return false;</div><div> </div><div>- // Only identify array mallocs for mallocs with 1 bitcast use. The unique </div><div>- // bitcast is needed to determine the type/size of the array allocation.</div><div>- if (!CI->hasOneUse()) return false;</div><div>+ const Type* T = getMallocAllocatedType(CI);</div><div> </div><div>- for (Value::use_const_iterator UI = CI->use_begin(), E = CI->use_end();</div><div>- UI != E; )</div><div>- if (!isa<BitCastInst>(cast<Instruction>(*UI++)))</div><div>- return false;</div><div>+ // We can only indentify an array malloc if we know the type of the malloc </div><div>+ // call.</div><div>+ if (!T) return false;</div><div> </div><div>- // malloc arg</div><div> Value* MallocArg = CI->getOperand(1);</div><div>- // element size</div><div>- const Type* T = getMallocAllocatedType(CI);</div><div>- if (!T) return false;</div><div> Constant *ElementSize = ConstantExpr::getSizeOf(T);</div><div>- </div><div>+ ElementSize = ConstantExpr::getTruncOrBitCast(ElementSize, </div><div>+ MallocArg->getType());</div><div>+ Constant *FoldedElementSize = ConstantFoldConstantExpression(</div><div>+ cast<ConstantExpr>(ElementSize), </div><div>+ Context, TD);</div><div>+</div><div>+</div><div> if (isa<ConstantExpr>(MallocArg))</div><div>- return (MallocArg == ElementSize) ? false : true;</div><div>+ return (MallocArg != ElementSize);</div><div> </div><div> BinaryOperator *BI = dyn_cast<BinaryOperator>(MallocArg);</div><div> if (!BI)</div><div> return false;</div><div> </div><div>- if (BI->getOpcode() != Instruction::Mul)</div><div>- return false;</div><div>+ if (BI->getOpcode() == Instruction::Mul) {</div><div>+ // ArraySize * ElementSize</div><div>+ if (BI->getOperand(1) == ElementSize ||</div><div>+ (FoldedElementSize && BI->getOperand(1) == FoldedElementSize))</div><div> </div><div>- if (BI->getOperand(1) != ElementSize)</div><div>+ return true;</div><div>+ </div><div> return false;</div><div>- </div><div>- return true;</div><div>+ }</div><div>+ </div><div>+ return false;</div><div> }</div><div> </div><div> /// getMallocType - Returns the PointerType resulting from the malloc call.</div><div>@@ -142,14 +163,24 @@</div><div> assert(isMalloc(CI) && "GetMallocType and not malloc call");</div><div> </div><div> const BitCastInst* BCI = NULL;</div><div>+ </div><div>+ // Determine if CallInst has a bitcast use.</div><div>+ for (Value::use_const_iterator UI = CI->use_begin(), E = CI->use_end();</div><div>+ UI != E; )</div><div>+ if ((BCI = dyn_cast<BitCastInst>(cast<Instruction>(*UI++))))</div><div>+ break;</div><div> </div><div>- // Determine type only if there is only 1 bitcast use of CI.</div><div>- if (CI->hasOneUse())</div><div>- for (Value::use_const_iterator UI = CI->use_begin(), E = CI->use_end();</div><div>- UI != E; )</div><div>- BCI = dyn_cast<BitCastInst>(cast<Instruction>(*UI++));</div><div>+ // Malloc call has 1 bitcast use and no other uses, so type is the bitcast's</div><div>+ // destination type.</div><div>+ if (BCI && CI->hasOneUse())</div><div>+ return cast<PointerType>(BCI->getDestTy());</div><div> </div><div>- return BCI ? reinterpret_cast<const PointerType*>(BCI->getDestTy()) : NULL;</div><div>+ // Malloc call was not bitcast, so the type is the malloc's return type, i8*.</div><div>+ if (!BCI)</div><div>+ return cast<PointerType>(CI->getType());</div><div>+</div><div>+ // Type could not be determined.</div><div>+ return NULL;</div><div> }</div><div><br></div></div><div>Also, I got rid of the unnecessary extractMallocCallFromBitCast if case while I was looking at this code.</div></div><div><br><blockquote type="cite"><div><br>Index: lib/Analysis/ValueTracking.cpp<br>===================================================================<br>--- lib/Analysis/ValueTracking.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 81898)<br>+++ lib/Analysis/ValueTracking.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(working copy)<br>@@ -20,6 +20,7 @@<br> #include "llvm/IntrinsicInst.h"<br> #include "llvm/LLVMContext.h"<br> #include "llvm/Operator.h"<br>+#include "llvm/Analysis/MallocHelper.h"<br> #include "llvm/Target/TargetData.h"<br> #include "llvm/Support/GetElementPtrTypeIterator.h"<br> #include "llvm/Support/MathExtras.h"<br>@@ -621,6 +622,24 @@<br> break;<br> }1<br> }<br>+ } else if (CallInst* CI = extractMallocCall(I)) {<br>+ unsigned Align = 0;<br>+ const Type* T = getMallocAllocatedType(CI);<br>+ if (TD && T) {<br>+ // Malloc returns maximally aligned memory.<br>+ Align = TD->getABITypeAlignment(T);<br>+ Align =<br>+ std::max(Align,<br>+ (unsigned)TD->getABITypeAlignment(<br>+ Type::getDoubleTy(V->getContext())));<br>+ Align =<br>+ std::max(Align,<br>+ (unsigned)TD->getABITypeAlignment(<br>+ Type::getInt64Ty(V->getContext())));<br>+ }<br>+ if (Align > 0)<br>+ KnownZero = Mask & APInt::getLowBitsSet(BitWidth,<br>+ CountTrailingZeros_32(Align));<br> }<br> break;<br> }<br><br>This is simply wrong; we can't make that guarantee for malloc.<br>Consider, for example, a malloc used as an SSE vector on Windows.<font class="Apple-style-span" color="#000000"><font class="Apple-style-span" color="#144FAE"><br></font></font></div></blockquote><div><br></div>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. </div><div><br></div><div>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.</div><div><br><blockquote type="cite"><div>More generally, getMallocAllocatedType seems a bit suspect in that an<br>i8 array will get messed up by the change to getMallocAllocatedType.<br></div></blockquote><div><br></div></div><div>See above.</div><div> </div><div><blockquote type="cite"><div><br>-Eli<br></div></blockquote></div><br></div></body></html>