[llvm-commits] [llvm] r84292 - in /llvm/trunk: examples/BrainF/ include/llvm/ lib/AsmParser/ lib/Bitcode/Reader/ lib/Transforms/IPO/ lib/Transforms/Scalar/ lib/VMCore/ test/Transforms/GlobalOpt/ test/Transforms/IndMemRem/ test/Transforms/InstCombine/

[Chris Lattner]

On Oct 21, 2009, at 12:12 PM, Victor Hernandez wrote:
>> 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.
>
> Fixed:

Much nicer, thanks.

>>> +    const Type* IntPtrTy = Type::getInt32Ty(Context);
>>> +    const Type* Int8PtrTy =  
>>> PointerType::getUnqual(Type::getInt8Ty(Context));
>>
>> 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.
>
> Fixed:
>   const Type *IntPtrTy = Type::getInt32Ty(Context);
>   const Type *Int8PtrTy = Type::getInt8PtrTy(Context);

Ok:

+  // Autoupgrade old malloc instruction to malloc call.
+  // FIXME: Remove in LLVM 3.0.
+  const Type *IntPtrTy = Type::getInt32Ty(Context);
+  const Type *Int8PtrTy = Type::getInt8PtrTy(Context);
+  if (!MallocF)
+    // Prototype malloc as "void *(int32)".
+    // This function is renamed as "malloc" in ValidateEndOfModule().
+    MallocF = cast<Function>(M->getOrInsertFunction(NULL, Int8PtrTy,
+                                                    IntPtrTy, NULL));
+  Inst = CallInst::CreateMalloc(BB, IntPtrTy, Ty, Size, MallocF);

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.

>
>>
>>> +    if (!MallocF)
>>> +      // Prototype malloc as "void *autoupgrade_malloc(int32)".
>>> +      MallocF = cast<Function>(M- 
>>> >getOrInsertFunction("autoupgrade_malloc",
>>> +                               Int8PtrTy, IntPtrTy, NULL));
>>
>> 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.
>
> Fixed.

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.

>>
>> 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.
>
> 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.

Looks good.

Thanks Victor,

-Chris
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20091021/f178e735/attachment.html>