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

[Victor Hernandez]

Evan,
Thanks for the feedback.

On Aug 13, 2009, at 12:42 AM, Evan Cheng wrote:

> Hi Victor,
>
> The patch looks very nice and clean. I do have some comments though:
>
> +//===- llvm/Analysis/Malloc.h ---------- Identify malloc calls --*-  
> C+
> + -*-===//
>
> I don't like Malloc.h. It seems too much like a system header. Perhaps
> MallocHelper.h? Also, I wonder if it belongs to include/llvm/Analysis.
> If all the utility functions are used within Analysis, perhaps it can
> go into lib/Analysis? Or perhaps under Transform/Util if it's only
> used by the optimizer?

You're right, that is a much better name.  The functions are used in  
both Analysis and Transform, so I don't know if I can move them there  
locally.  I will look into it.

>
> +//
> =
> =
> = 
> ----------------------------------------------------------------------
> ===//
> +//  malloc Call Utility Functions.
> +//
> +
> +/// IsMalloc - Returns the corresponding CallInst if the instruction
> is a call
> +/// to malloc and all its uses are BitCastInst.  This matches the IR
> generated
> +/// by CallInst::CreateMalloc().  Since CreateMallocCall() only
> creates calls,
> +/// we ignore InvokeInst here.
> +CallInst* IsMalloc(Instruction* I);
>
> This seems to be checking a property (all uses are BitCastInst) that's
> not apparent from the function name. Why do we want to check whether
> uses are BitCast's?

The bitcast usage assumption was to facilitate the malloc type and  
array size helper functions.  But in fact this assumption should be  
part of those functions, and in fact shouldn't be made at all as I am  
finding that there are malloc calls that result in non-bit cast uses,  
and I need to handle that case as well.
>
> +/// GetMallocCall - Returns the malloc call whose result is being  
> bit-
> casted.
> +CallInst* llvm::GetMallocCall(BitCastInst* BCI) {
> +  if (CallInst* CI = dyn_cast<CallInst>(BCI->getOperand(0))) {
> +    assert(IsMalloc(CI) && "GetMallocCall and not malloc call");
> +    return CI;
> +  }
> +
> +  return NULL;
> +}
>
> Ah, I didn't quite understand this when I looked at the declaration.
> It looks like the caller must know it's either a bitcast from a non-
> call (then it return null) or it's a call to malloc. That seems a bit
> strange. Could it just return null if it's a call, but not to malloc?
> This seems like a local utility routine that belongs in a .cpp file
> rather than being made available globally. It performs a very narrow
> and specialized function.

You are right, I can move it to the few places it gets used.  And also  
have it return NULL more consistently.

>
> +void llvm::ReplaceAndEraseMalloc(CallInst* CI, Value* newVal) {
> +  for (Value::use_iterator UI = CI->use_begin(), E = CI->use_end();
> UI != E; ) {
> +    Instruction* I = cast<Instruction>(*UI++);
> +    I->replaceAllUsesWith(newVal);
> +    RecursivelyDeleteTriviallyDeadInstructions(I);
> +  }
> +}
>
> The function name indicates it's specific to malloc but it's
> applicable to any call instruction, right? Do we really need it? It
> seems we might have existing routine that does this already?

I need to find that existing routine.  It is not specific to mallocs  
or calls, so if it does not exist, I'll put it in a more generic place.
>
> +/// EraseMalloc - For all uses of the malloc call (which should all  
> be
> +/// BitCastInsts), recursively delete all of the dead malloc
> instructions (all
> +/// the bitcasts, the malloc call, and the instructions that computed
> the
> +/// malloc's size argument).  This function is defined in Local.cpp
> because it
> +/// uses llvm::RecursivelyDeleteTriviallyDeadInstructions().
> +void llvm::EraseMalloc(CallInst* CI) {
>
> Same concern. If we do need this, then it needs to at least assert if
> the call isn't to malloc. Do you have subsequent patch that uses this?
> Can it become a local function?

This is used only by GlobalOpt, and if I don't find an existing  
routing for it and the one above, I'll make it local to that file.

Victor

>
> Thanks,
>
> Evan
>
> 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().
>>
>> <mallocUtils.diff>
>>
>> Victor
>>
>> ---
>> Victor Hernandez          vhernandez at apple.com
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits