[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/

[Victor Hernandez]

I just made your suggested changes in rev 84919.

Victor

On Oct 21, 2009, at 5:38 PM, Chris Lattner wrote:

> 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/20091022/78712fe5/attachment.html>