<html><head></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; ">Chris,<div><br></div><div>I fixed all your suggestions in rev 84772.</div><div><br></div><div><div><div>On Oct 17, 2009, at 10:24 PM, Chris Lattner wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div>On Oct 16, 2009, at 5:00 PM, Victor Hernandez wrote:<br><blockquote type="cite">URL: <a href="http://llvm.org/viewvc/llvm-project?rev=84292&view=rev">http://llvm.org/viewvc/llvm-project?rev=84292&view=rev</a><br></blockquote><blockquote type="cite">Log:<br></blockquote><blockquote type="cite">Autoupgrade malloc insts to malloc calls.<br></blockquote><blockquote type="cite">Update testcases that rely on malloc insts being present.<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">Also prematurely remove MallocInst handling from IndMemRemoval and RaiseAllocations to help pass tests in this incremental step.<br></blockquote><br>Thanks for working on this Victor. I committed a few patches to do random cleanups, here are some more things for you to look at:<br><br><blockquote type="cite">+++ llvm/trunk/lib/AsmParser/LLParser.cpp Fri Oct 16 19:00:19 2009<br></blockquote><blockquote type="cite">@@ -69,6 +69,27 @@<br></blockquote><blockquote type="cite">/// ValidateEndOfModule - Do final validity and sanity checks at the end of the<br></blockquote><blockquote type="cite">/// module.<br></blockquote><blockquote type="cite">bool LLParser::ValidateEndOfModule() {<br></blockquote><blockquote type="cite">+ // Update auto-upgraded malloc calls from "autoupgrade_malloc" to "malloc".<br></blockquote><blockquote type="cite">+ if (MallocF) {<br></blockquote><blockquote type="cite">+ MallocF->setName("malloc");<br></blockquote><blockquote type="cite">+ // If setName() does not set the name to "malloc", then there is already a<br></blockquote><blockquote type="cite">+ // declaration of "malloc". In that case, iterate over all calls to MallocF<br></blockquote><blockquote type="cite">+ // and get them to call the declared "malloc" instead.<br></blockquote><blockquote type="cite">+ if (MallocF->getName() != "malloc") {<br></blockquote><blockquote type="cite">+ Function* realMallocF = M->getFunction("malloc");<br></blockquote><blockquote type="cite">+ for (User::use_iterator UI = MallocF->use_begin(), UE= MallocF->use_end();<br></blockquote><blockquote type="cite">+ UI != UE; ) {<br></blockquote><blockquote type="cite">+ User* user = *UI;<br></blockquote><blockquote type="cite">+ UI++;<br></blockquote><blockquote type="cite">+ if (CallInst *Call = dyn_cast<CallInst>(user))<br></blockquote><blockquote type="cite">+ Call->setCalledFunction(realMallocF);<br></blockquote><blockquote type="cite">+ }<br></blockquote><br>This should use V->replaceAllUsesWith() instead of manually walking the use list. Doing this will handle non-call users as well as call users. What happens if the prototypes don't agree for these two functions? I think you'll get an abort. Please fix by using a constantexpr bitcast to the expected type.<br></div></blockquote><div><br></div>Fixed:</div><div><br></div><div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 11px/normal Menlo; color: rgb(0, 132, 0); "><font class="Apple-style-span" color="#000000"><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 11px/normal Menlo; color: rgb(0, 139, 0); "><span style="color: #000000"> </span>// Update auto-upgraded malloc calls to "malloc".</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 11px/normal Menlo; color: rgb(0, 139, 0); "><span style="color: #000000"> </span>// FIXME: Remove in LLVM 3.0.</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 11px/normal Menlo; "> <span style="color: #cd00a3">if</span> (MallocF) {</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 11px/normal Menlo; "> MallocF->setName(<span style="color: #e50000">"malloc"</span>);</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 11px/normal Menlo; color: rgb(0, 139, 0); "><span style="color: #000000"> </span>// If setName() does not set the name to "malloc", then there is already a </div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 11px/normal Menlo; color: rgb(0, 139, 0); "><span style="color: #000000"> </span>// declaration of "malloc". In that case, iterate over all calls to MallocF</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 11px/normal Menlo; color: rgb(0, 139, 0); "><span style="color: #000000"> </span>// and get them to call the declared "malloc" instead.</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 11px/normal Menlo; "> <span style="color: #cd00a3">if</span> (MallocF->getName() != <span style="color: #e50000">"malloc"</span>) {</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 11px/normal Menlo; "> Constant* RealMallocF = M->getFunction(<span style="color: #e50000">"malloc"</span>);</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 11px/normal Menlo; "> <span style="color: #cd00a3">if</span> (RealMallocF->getType() != MallocF->getType())</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 11px/normal Menlo; "> RealMallocF = ConstantExpr::getBitCast(RealMallocF, MallocF->getType());</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 11px/normal Menlo; "> MallocF->replaceAllUsesWith(RealMallocF);</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 11px/normal Menlo; "> MallocF->eraseFromParent();</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 11px/normal Menlo; "> MallocF = <span style="color: #cd00a3">NULL</span>;</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 11px/normal Menlo; "> }</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 11px/normal Menlo; "> }</div><div><br></div></font></div></div><div><br><blockquote type="cite"><div><br><blockquote type="cite">+ if (!realMallocF->doesNotAlias(0)) realMallocF->setDoesNotAlias(0);<br></blockquote><br>I don't think you have to care about this. We don't care that old code be performant, just that it work.<br></div></blockquote><div><br></div>See above.<br><blockquote type="cite"><div><br><blockquote type="cite">@@ -3466,10 +3487,21 @@<br></blockquote><blockquote type="cite"> if (Size && Size->getType() != Type::getInt32Ty(Context))<br></blockquote><blockquote type="cite"> return Error(SizeLoc, "element count must be i32");<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">- if (Opc == Instruction::Malloc)<br></blockquote><blockquote type="cite">- Inst = new MallocInst(Ty, Size, Alignment);<br></blockquote><blockquote type="cite">- else<br></blockquote><blockquote type="cite">+ if (isAlloca)<br></blockquote><blockquote type="cite"> Inst = new AllocaInst(Ty, Size, Alignment);<br></blockquote><br>Please use an early exit here to unnest the 'else'.<br></div></blockquote><div><br></div>Fixed.</div><div><br></div><div><blockquote type="cite"><div><br><blockquote type="cite">+ else {<br></blockquote><blockquote type="cite">+ // Autoupgrade old malloc instruction to malloc call.<br></blockquote><br>Please add a FIXME to remove this in LLVM 3.0 like I added to other places.<br></div></blockquote><div><br></div>Fixed.</div><div><br><blockquote type="cite"><div><br><blockquote type="cite">+ const Type* IntPtrTy = Type::getInt32Ty(Context);<br></blockquote><blockquote type="cite">+ const Type* Int8PtrTy = PointerType::getUnqual(Type::getInt8Ty(Context));<br></blockquote><br>Please use 'const Type *IntPtrTy', watch the * placement. There is now a helper function that you can use instead of 'PointerType::getUnqual(Type::getInt8Ty(Context));', please use it.<br></div></blockquote><div><br></div>Fixed:</div><div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 11px/normal Menlo; "> <span style="color: #bb2ca2">const</span> Type *IntPtrTy = Type::getInt32Ty(Context);</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 11px/normal Menlo; "> <span style="color: #bb2ca2">const</span> Type *Int8PtrTy = Type::getInt8PtrTy(Context);</div><div><font class="Apple-style-span" face="Menlo" size="3"><span class="Apple-style-span" style="font-size: 11px; "><br></span></font></div><blockquote type="cite"><div><br><blockquote type="cite">+ if (!MallocF)<br></blockquote><blockquote type="cite">+ // Prototype malloc as "void *autoupgrade_malloc(int32)".<br></blockquote><blockquote type="cite">+ MallocF = cast<Function>(M->getOrInsertFunction("autoupgrade_malloc",<br></blockquote><blockquote type="cite">+ Int8PtrTy, IntPtrTy, NULL));<br></blockquote><br>Why do you bother naming autoupgrade_malloc? This is in the user's namespace, so it can conflict with a function named autoupgrade_malloc in the user's code. Please just leave the name empty, making it impossible for it to conflict.<br></div></blockquote><div><br></div>Fixed.</div><div><br><blockquote type="cite"><div><br><blockquote type="cite">+ // "autoupgrade_malloc" updated to "malloc" in ValidateEndOfModule().<br></blockquote><blockquote type="cite">+<br></blockquote><blockquote type="cite">+ Inst = cast<Instruction>(CallInst::CreateMalloc(BB, IntPtrTy, Ty,<br></blockquote><blockquote type="cite">+ Size, MallocF));<br></blockquote></div></blockquote><div><br></div>Fixed.</div><div><br><blockquote type="cite"><div><br>CreateMalloc doesn't return something convertible to Instruction? Why do you need the cast<>?<br></div></blockquote><div><br></div>Fixed.</div><div><br><blockquote type="cite"><div><br><blockquote type="cite">+++ llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp Fri Oct 16 19:00:19 2009<br></blockquote><blockquote type="cite">@@ -2044,14 +2044,21 @@<br></blockquote><blockquote type="cite"> }<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite"> case bitc::FUNC_CODE_INST_MALLOC: { // MALLOC: [instty, op, align]<br></blockquote><blockquote type="cite">+ // Autoupgrade malloc instruction to malloc call.<br></blockquote><br>FIXME remove in 3.0.<br></div></blockquote><div><br></div>Fixed.</div><div><br><blockquote type="cite"><div><br><blockquote type="cite"> if (Record.size() < 3)<br></blockquote><blockquote type="cite"> return Error("Invalid MALLOC record");<br></blockquote><blockquote type="cite"> const PointerType *Ty =<br></blockquote><blockquote type="cite"> dyn_cast_or_null<PointerType>(getTypeByID(Record[0]));<br></blockquote><blockquote type="cite"> Value *Size = getFnValueByID(Record[1], Type::getInt32Ty(Context));<br></blockquote><blockquote type="cite">- unsigned Align = Record[2];<br></blockquote><blockquote type="cite"> if (!Ty || !Size) return Error("Invalid MALLOC record");<br></blockquote><blockquote type="cite">- I = new MallocInst(Ty->getElementType(), Size, (1 << Align) >> 1);<br></blockquote><blockquote type="cite">+ if (!CurBB) return Error("Invalid malloc instruction with no BB");<br></blockquote><blockquote type="cite">+ const Type* Int32Ty = IntegerType::getInt32Ty(CurBB->getContext());<br></blockquote><br>* placement.<br></div></blockquote><div><br></div>Fixed.</div><div><br><blockquote type="cite"><div><br><blockquote type="cite">+ if (Size->getType() != Int32Ty)<br></blockquote><blockquote type="cite">+ Size = CastInst::CreateIntegerCast(Size, Int32Ty, false /*ZExt*/,<br></blockquote><blockquote type="cite">+ "", CurBB);<br></blockquote><br>How is this possible? Size has to be int32 per the code above.<br></div></blockquote><div><br></div>It is not possible, copy-and-past error from other calls to <span class="Apple-style-span" style="font-family: Menlo; font-size: 11px; ">CallInst::CreateMalloc(). Deleted the IntegerCast.</span><blockquote type="cite"><div><br><br><blockquote type="cite">+ Value* Malloc = CallInst::CreateMalloc(CurBB, Int32Ty,<br></blockquote><blockquote type="cite">+ Ty->getElementType(), Size, NULL);<br></blockquote><blockquote type="cite">+ I = cast<Instruction>(Malloc);<br></blockquote><br>again, why the cast?<br></div></blockquote><div><br></div>Fixed.</div><div><br><blockquote type="cite"><div><br><blockquote type="cite">+++ llvm/trunk/lib/Transforms/IPO/IndMemRemoval.cpp Fri Oct 16 19:00:19 2009<br></blockquote><blockquote type="cite">@@ -24,6 +24,7 @@<br></blockquote><br>I removed this pass, it was specifically their to support malloc/free instructions, it is not needed anymore (and not used in any case).<br></div></blockquote><div><br></div>Thanks.</div><div><br><blockquote type="cite"><div><br><blockquote type="cite">+++ llvm/trunk/lib/Transforms/IPO/RaiseAllocations.cpp Fri Oct 16 19:00:19 2009<br></blockquote><blockquote type="cite">@@ -1,4 +1,4 @@<br></blockquote><blockquote type="cite">-//===- RaiseAllocations.cpp - Convert @malloc & @free calls to insts ------===//<br></blockquote><blockquote type="cite">+//===- RaiseAllocations.cpp - Convert @free calls to insts ------===//<br></blockquote><br>Hopefully your future changes to remove FreeInst will completely zap this pass.<br></div></blockquote><div><br></div>Yup, that is next.</div><div><br><blockquote type="cite"><div><br><blockquote type="cite">+++ llvm/trunk/lib/Transforms/Scalar/Reassociate.cpp Fri Oct 16 19:00:19 2009<br></blockquote><blockquote type="cite">@@ -29,6 +29,7 @@<br></blockquote><blockquote type="cite">#include "llvm/IntrinsicInst.h"<br></blockquote><blockquote type="cite">#include "llvm/LLVMContext.h"<br></blockquote><blockquote type="cite">#include "llvm/Pass.h"<br></blockquote><blockquote type="cite">+#include "llvm/Analysis/MallocHelper.h"<br></blockquote><blockquote type="cite">#include "llvm/Assembly/Writer.h"<br></blockquote><blockquote type="cite">#include "llvm/Support/CFG.h"<br></blockquote><blockquote type="cite">#include "llvm/Support/Debug.h"<br></blockquote><blockquote type="cite">@@ -121,7 +122,7 @@<br></blockquote><blockquote type="cite"> if (I->getOpcode() == Instruction::PHI ||<br></blockquote><blockquote type="cite"> I->getOpcode() == Instruction::Alloca ||<br></blockquote><blockquote type="cite"> I->getOpcode() == Instruction::Load ||<br></blockquote><blockquote type="cite">- I->getOpcode() == Instruction::Malloc ||<br></blockquote><blockquote type="cite">+ I->getOpcode() == Instruction::Malloc || isMalloc(I) ||<br></blockquote><blockquote type="cite"> I->getOpcode() == Instruction::Invoke ||<br></blockquote><blockquote type="cite"> (I->getOpcode() == Instruction::Call &&<br></blockquote><blockquote type="cite"> !isa<DbgInfoIntrinsic>(I)) ||<br></blockquote><br>A check for isMalloc isn't needed here at all, because call is handled. Please remove all the malloc logic and the #include.<br></div></blockquote><div><br></div>Fixed.</div><div><br><blockquote type="cite"><div><br><blockquote type="cite">+++ llvm/trunk/test/Transforms/InstCombine/cast-malloc.ll Fri Oct 16 19:00:19 2009<br></blockquote><blockquote type="cite">@@ -1,6 +1,6 @@<br></blockquote><br>I removed this, it is now pointless. You convert it to test that instcombine isn't doing anything.<br></div></blockquote><div><br></div>OK.</div><div><br><blockquote type="cite"><div><br><blockquote type="cite"><br></blockquote><blockquote type="cite">==============================================================================<br></blockquote><blockquote type="cite">--- llvm/trunk/test/Transforms/InstCombine/malloc2.ll (original)<br></blockquote><blockquote type="cite">+++ llvm/trunk/test/Transforms/InstCombine/malloc2.ll Fri Oct 16 19:00:19 2009<br></blockquote><blockquote type="cite">@@ -1,5 +1,4 @@<br></blockquote><blockquote type="cite">; RUN: opt < %s -instcombine -S | grep {ret i32 0}<br></blockquote><blockquote type="cite">-; RUN: opt < %s -instcombine -S | not grep malloc<br></blockquote><blockquote type="cite">; PR1313<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">define i32 @test1(i32 %argc, i8* %argv, i8* %envp) {<br></blockquote><br><br>This test is being miscompiled now. This is a serious bug that one of your patches has introduced. We're now compiling (e.g.):<br><br>define i32 @test1(i32 %argc, i8* %argv, i8* %envp) {<br> %tmp15.i.i.i23 = malloc [2564 x i32] ; <[2564 x i32]*> [#uses=1]<br> %c = icmp eq [2564 x i32]* %tmp15.i.i.i23, null ; <i1>:0 [#uses=1]<br> %retval = zext i1 %c to i32 ; <i32> [#uses=1]<br> ret i32 %retval<br>}<br><br>into:<br><br>define i32 @test1(i32 %argc, i8* %argv, i8* %envp) {<br> %malloccall = tail call i8* @malloc(i32 ptrtoint ([2564 x i32]* getelementptr ([2564 x i32]* null, i32 1) to i32)) ; <i8*> [#uses=0]<br> ret i32 0<br>}<br><br>The old code was correct because it would delete the call to malloc, so it is safe to assume that the malloc (which never got run) never ran out of space. The transformation that does this should just be removed.</div></blockquote><br></div><div>I fixed InstCombine so that the icmp is removed only if the malloc call is removed (which requires explicit removal because the Worklist won't DCE any calls since they can have side-effects). I also updated the testcase to use FileCheck.</div><div><br></div><div>Victor</div><div><br></div><div><br></div><br></div></body></html>