[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

[Duncan Sands]

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?

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

> +///  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?

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

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

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

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

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

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.

Ciao,

Duncan.