[llvm-commits] [PATCH] Add malloc call utility functions

Victor Hernandez vhernandez at apple.com
Thu Aug 13 10:47:17 PDT 2009


Eric,

Thanks for the feedback.

On Aug 12, 2009, at 11:42 AM, Eric Christopher wrote:

>
> On Aug 11, 2009, at 7:31 PM, Victor Hernandez wrote:
>
>> I am working on fixing the MallocInst/i64 alloca bug:
>> http://llvm.org/bugs/show_bug.cgi?id=715
>>
>> Here is the first of a series of patches that will result in tearing
>> out MallocInst.  Before I can tear it out, I need to update all of
>> the optimization passes and transforms that operate on MallocInst to
>> operate on malloc CallInst instead.
>>
>> This patch consists of a set of utility functions that create IR for
>> malloc calls and identify that IR.  It also rewrites
>> LowerAllocations to use the new utility function,
>> CallInst::CreateMalloc().
>
>
> Looks pretty good!
>
> Have a few questions here:
>
> +/// IsMalloc - Returns the corresponding CallInst if the instruction
> is a call
> +/// to malloc and all its uses are BitCastInst.  This matches the IR
> generated
>
> ... Why does this routine care about the uses in determining if it's a
> call to malloc?

The malloc helper functions rely on the bit cast uses to determine the  
type being allocated.  The type is used by GlobalOpt, and is also  
needed to determine if the malloc is for an array malloc, a piece of  
information also needed by GlobalOpt.  But the bit cast use issue only  
affects the type and arraySize helper functions for the malloc call,  
so I will move the bitcast use assumption to those functions.  Also, I  
am finding that there are mallocs whose uses are not all bitcasts, and  
I need to be conservative and still handle that case, by using the  
declared return type of malloc.

>
> +  Value::use_iterator UI = CI->use_begin();
> +  return cast<BitCastInst>(*(CI->use_begin()));
>
> Is this why? :) Seems that an assert would be better, but I worry
> about a function that does something like:
>
> void* myMalloc(size_t s)
> {
>      // Log how many bytes we're allocating...
>
>     // Call malloc
>     return malloc(s);
> }
>
> I guess here we've got a bitcast from i8* to void* due to the
> prototype for malloc in IR.
>
> That said, I'm not sure why you need this function. Wouldn't it just
> be the first use with the bitcast invariant?

As I said above, I am moving the bitcast-checking to the malloc type  
and array size helper functions.

>
> +/// by CallInst::CreateMalloc().  Since CreateMallocCall() only
> creates calls,
> +/// we ignore InvokeInst here.
>
> This is because malloc itself never threw but routines that called it
> do I imagine? Oh and you've got CreateMallocCall in a few places and
> CreateMalloc in others.  CreateMallocCall may be more descriptive
> since you've got the invariant that there's always going to be a cast
> associated, but I sometimes like my bikesheds to be green instead of
> blue.

I prefer CreateMalloc because it generates more than just the call.

>
> +  assert(IsConstantOne(ElementSize) ||
> +         isa<Constant>(MallocArg) ||
> +         isa<BinaryOperator>(MallocArg) &&
> +         "GetMallocArraySize and malformed malloc IR");
>
> Here I'm not sure about the standard practice but this might be nice
> to break out into different and more helpful asserts.

I will look into spreading these out into 3 separate asserts.

>
> -eric
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits




More information about the llvm-commits mailing list