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

[Eric Christopher]

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?

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

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

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

-eric