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

[Evan Cheng]

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?

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

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

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

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

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