<html><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><div>On Oct 21, 2009, at 12:12 PM, Victor Hernandez wrote:</div><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><div><blockquote type="cite"><div>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></div></blockquote><div><br></div>Much nicer, thanks.</div><div><br></div><div><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><div><blockquote type="cite"><div><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></div></div></blockquote><div><br></div><div>Ok:</div><div><br></div><div><span class="Apple-style-span" style="font-family: monospace; ">+ // Autoupgrade old malloc instruction to malloc call.<br>+ // FIXME: Remove in LLVM 3.0.<br>+ const Type *IntPtrTy = Type::getInt32Ty(Context);<br>+ const Type *Int8PtrTy = Type::getInt8PtrTy(Context);<br>+ if (!MallocF)<br>+ // Prototype malloc as "void *(int32)".<br>+ // This function is renamed as "malloc" in ValidateEndOfModule().<br>+ MallocF = cast<Function>(M->getOrInsertFunction(NULL, Int8PtrTy, <br>+ IntPtrTy, NULL));<br>+ Inst = CallInst::CreateMalloc(BB, IntPtrTy, Ty, Size, MallocF);<br></span></div><div><font class="Apple-style-span" face="monospace"><br></font></div><div><font class="Apple-style-span" face="monospace">It doesn't matter because this is very cold code, but you might as well sink the 'get' of Int8PtrTy into the 'if' for tidiness.</font></div><br><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><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></div></blockquote><div><br></div><div>Thanks. While passing in NULL to getOrInsertFunction will probably work, please pass in "" instead. This makes it obvious that it is the name you are specifying.</div><div><br></div><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><div><blockquote type="cite"><div><font class="Apple-style-span" color="#000000"><br></font>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></div></blockquote></div><br><div>Looks good.</div><div><br></div><div>Thanks Victor,</div><div><br></div><div>-Chris</div></body></html>