[llvm-commits] [llvm] r84199 - in /llvm/trunk: include/llvm/Analysis/MallocHelper.h lib/Analysis/MallocHelper.cpp lib/Analysis/PointerTracking.cpp lib/Transforms/IPO/GlobalOpt.cpp

[Victor Hernandez]

On Oct 16, 2009, at 12:03 AM, Duncan Sands wrote:

> Hi Victor, there doesn't seem to be a testcase.
>
>> The fix is to classify malloc's into 3 categories:
>> 1. non-array mallocs
>> 2. array mallocs whose array size can be determined
>> 3. mallocs that cannot be determined to be of type 1 or 2 and  
>> cannot be optimized
>
> Do you plan to do anything in the case when a variable amount is
> being allocated, and it can be proved that the amount is a multiple
> of the size of the allocated type?

Yes, I'd like to handle that case.
I'd also like to handle the case where the malloc argument is produced  
by shifts and/or adds.

>
>> -/// getMallocArraySize - Returns the array size of a malloc call.   
>> The array
>> -/// size is computated in 1 of 3 ways:
>> -///  1. If the element type if of size 1, then array size is the  
>> argument to +/// getMallocArraySize - Returns the array size of a  
>> malloc call.  For array
>> +/// mallocs, the size is computated in 1 of 3 ways:
>
> computated -> computed

Will fix that.

>
>> +///  1. If the element type is of size 1, then array size is the  
>> argument to  ///     malloc.
>> ///  2. Else if the malloc's argument is a constant, the array size  
>> is that
>> ///     argument divided by the element type's size.
>
> What if the constant is not a multiple of the size of the malloc'd  
> type?

I had not special-cased that case, but I think the current code  
properly does not treat that as an optimizeable array malloc.  It  
might miss some examples, but that seems like a reasonable edge case  
to ignore.  Or is there a common situation where such mallocs can occur?

>
>> ///  3. Else the malloc argument must be a multiplication and the  
>> array size is
>> ///     the first operand of the multiplication.
>
> So you are not caring about the multiplication overflowing and giving
> something funny?

I am not checking for that.  Need to think how to do that.

>
>> -/// This function returns constant 1 if:
>> -///  1. The malloc call's allocated type cannot be determined.
>> -///  2. IR wasn't created by a call to CallInst::CreateMalloc()  
>> with a non-NULL
>> -///     ArraySize.
>> +/// For non-array mallocs, the computed size is constant 1. +///  
>> This function returns NULL for all mallocs whose array size cannot be
>> +/// determined.
>
> Just a general comment about writing comments: I don't think the  
> comment
> should be explaining the algorithm getMallocArraySize uses, it  
> should be
> explaining the semantics of getMallocArraySize.  Can't you just say  
> that
> if the amount malloc'd is a multiple of the size of the malloced type
> then it returns that multiple, otherwise it returns NULL?

Good point.  Will simplify the comment.

>
>> +/// isSafeToGetMallocArraySize - Returns true if the array size of  
>> a malloc can
>> +/// be determined.  It can be determined in these 3 cases of  
>> malloc codegen:
>> +/// 1. non-array malloc: The malloc's size argument is a constant  
>> and equals the ///    size of the type being malloced.
>
> The above line should be two lines.

Thanks.

>
>> +/// 2. array malloc: This is a malloc call with one bitcast use  
>> AND the malloc
>> +///    call's size argument is a constant multiple of the size of  
>> the malloced
>> +///    type.
>
> Rather than talking about one bitcast use, why don't you say: we were
> able to determine the type being malloced.

OK.

>
>> +/// 3. array malloc: This is a malloc call with one bitcast use  
>> AND the malloc
>> +///    call's size argument is the result of a multiplication by  
>> the size of the
>> +///    malloced type.
>> +/// Otherwise returns false.
>
> As I mentioned above, I don't think you should describe the  
> algorithm a
> function uses in the comment describing it.  Why not only say:  
> Returns true if the array size of a malloc can be determined.

OK.

>
> Also, why have this logic in its own function?  It is only used in
> getMallocArraySize, and there is the danger that the two functions
> get out of sync.

Good point, I thought it read easier factored out this way, but I can  
change it to ensure it being in sync.

>
> Ciao,
>
> Duncan.

Thanks for the review.

Victor