[llvm-commits] [llvm] r82300 - in /llvm/trunk/lib/Transforms: IPO/FunctionAttrs.cpp IPO/GlobalOpt.cpp Scalar/GVN.cpp Scalar/InstructionCombining.cpp Scalar/Reassociate.cpp Scalar/SCCP.cpp Scalar/SimplifyLibCalls.cpp Scalar/TailDuplication.cpp Utils/InlineCost.cpp

Chris Lattner clattner at apple.com
Sun Sep 27 16:49:02 CDT 2009


On Sep 18, 2009, at 3:35 PM, Victor Hernandez wrote:
> URL: http://llvm.org/viewvc/llvm-project?rev=82300&view=rev
> Log:
> Enhance transform passes so that they apply the same tranforms to  
> malloc calls as to MallocInst.

Great!  Despite the number of comments below, this is really good work  
and a nice cleanup.  When is MallocInst going to get removed?


> +++ llvm/trunk/lib/Transforms/IPO/FunctionAttrs.cpp Fri Sep 18  
> 17:35:49 2009
> @@ -152,8 +153,8 @@
>         // Writes memory.  Just give up.
>         return false;
>
> -      if (isa<MallocInst>(I))
> -        // MallocInst claims not to write memory!  PR3754.
> +      if (isa<MallocInst>(I) || isMalloc(I))
> +        // malloc claims not to write memory!  PR3754.
>         return false;

This isn't needed, malloc calls are not readonly.  I removed it.

>       // If this instruction may read memory, remember that.
> @@ -247,8 +248,11 @@
>     if (Instruction *RVI = dyn_cast<Instruction>(RetVal))
>       switch (RVI->getOpcode()) {
>         // Extend the analysis by looking upwards.
> -        case Instruction::GetElementPtr:
>         case Instruction::BitCast:
> +          if (isMalloc(RVI))
> +            break;

This is not needed, the next time through the loop the call will get  
considered.  I removed it.

> +          // fall through
> +        case Instruction::GetElementPtr:
>           FlowsToReturn.insert(RVI->getOperand(0));
>           continue;
>         case Instruction::Select: {
> @@ -267,6 +271,8 @@
>         case Instruction::Malloc:
>           break;
>         case Instruction::Call:
> +          if (isMalloc(RVI))
> +            break;


This *shouldn't* be needed.  The result of the malloc prototype should  
already be marked NoAlias.  Please verify that this is happening  
(likewise for calloc, valloc, memalign, etc).  If not, llvm-gcc/clang  
and simplifylibcalls should be updated to make this happen.  One bonus  
of this that more stuff will benefit.

I see that you already modified SimplifyLibCalls.cpp to handle malloc,  
so this can probably be removed.

> +++ llvm/trunk/lib/Transforms/IPO/GlobalOpt.cpp Fri Sep 18 17:35:49  
> 2009
> @@ -24,6 +24,7 @@
> #include "llvm/Module.h"
> #include "llvm/Pass.h"
> #include "llvm/Analysis/ConstantFolding.h"
> +#include "llvm/Analysis/MallocHelper.h"
> #include "llvm/Target/TargetData.h"
> #include "llvm/Support/CallSite.h"
> #include "llvm/Support/Compiler.h"


This all seems reasonable.  Please delete all the corresponding code  
that handles MallocInst though when it goes away :)

> = 
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> ======================================================================
> --- llvm/trunk/lib/Transforms/Scalar/GVN.cpp (original)
> @@ -1393,7 +1394,7 @@
>
>   // Allocations are always uniquely numbered, so we can save time  
> and memory
>   // by fast failing them.
> -  } else if (isa<AllocationInst>(I) || isa<TerminatorInst>(I)) {
> +  } else if (isa<AllocationInst>(I) || isMalloc(I) ||  
> isa<TerminatorInst>(I)) {

This check isn't necessary, it was an optimize for a quick fail, but  
isMalloc is more intensive than the previous isa call, so I just  
removed it.

> @@ -1558,8 +1559,8 @@
>          BE = CurrentBlock->end(); BI != BE; ) {
>       Instruction *CurInst = BI++;
>
> +      if (isa<AllocationInst>(CurInst) || isMalloc(CurInst) ||
> +          isa<TerminatorInst>(CurInst) || isa<PHINode>(CurInst) ||
>           (CurInst->getType() == Type::getVoidTy(F.getContext())) ||
>           CurInst->mayReadFromMemory() || CurInst->mayHaveSideEffects 
> () ||
>           isa<DbgInfoIntrinsic>(CurInst))

malloc calls returns true for mayReadFromMemory/mayHaveSideEffects, so  
this check is not needed, removed.

> = 
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> ======================================================================
> --- llvm/trunk/lib/Transforms/Scalar/InstructionCombining.cpp  
> (original)
> @@ -5891,9 +5893,9 @@
>
>   // icmp <global/alloca*/null>, <global/alloca*/null> - Global/ 
> Stack value
>   // addresses never equal each other!  We already know that Op0 !=  
> Op1.
> -  if ((isa<GlobalValue>(Op0) || isa<AllocaInst>(Op0) ||
> +  if ((isa<GlobalValue>(Op0) || isa<AllocaInst>(Op0) || isMalloc 
> (Op0) ||
>        isa<ConstantPointerNull>(Op0)) &&
> -      (isa<GlobalValue>(Op1) || isa<AllocaInst>(Op1) ||
> +      (isa<GlobalValue>(Op1) || isa<AllocaInst>(Op1) || isMalloc 
> (Op1) ||
>        isa<ConstantPointerNull>(Op1)))

Instead of special casing malloc here, please handle all "noalias"  
functions the same.

> @@ -6231,8 +6233,33 @@
...

> +      case Instruction::BitCast:
> +        // If we have (malloc != null), and if the malloc has a  
> single use, we
> +        // can assume it is successful and remove the malloc.
> +        CallInst* CI = extractMallocCallFromBitCast(LHSI);
> +        if (CI && CI->hasOneUse() && LHSI->hasOneUse()
> +            && isa<ConstantPointerNull>(RHSC)) {
> +          Worklist.Add(LHSI);
> +          Worklist.Add(CI);
> +          return ReplaceInstUsesWith(I,
> +                                     ConstantInt::get 
> (Type::getInt1Ty(*Context),
> +                                                      ! 
> I.isTrueWhenEqual()));
>         }

The bitcast case is not needed here: instcombine turns icmp(bitcast 
(x), null) -> icmp(x, null) already.  Removed.

> @@ -9796,7 +9826,7 @@
>     TerminatorInst *TI = II->getParent()->getTerminator();
>     bool CannotRemove = false;
>     for (++BI; &*BI != TI; ++BI) {
> -      if (isa<AllocaInst>(BI)) {
> +      if (isa<AllocaInst>(BI) || isMalloc(BI)) {
>         CannotRemove = true;
>         break;
>       }

This is fine, but is a new optimization, not preserving an old one.

> @@ -11060,7 +11090,8 @@
>       if (Offset == 0) {
>         // If the bitcast is of an allocation, and the allocation  
> will be
>         // converted to match the type of the cast, don't touch this.
> -        if (isa<AllocationInst>(BCI->getOperand(0))) {
> +        if (isa<AllocationInst>(BCI->getOperand(0)) ||
> +            isMalloc(BCI->getOperand(0))) {
>           // See if the bitcast simplifies, if so, don't nuke this  
> GEP yet.
>           if (Instruction *I = visitBitCast(*BCI)) {
>             if (I != BCI) {
> @@ -11191,6 +11222,21 @@
>       EraseInstFromFunction(FI);
>       return EraseInstFromFunction(*MI);
>     }
> +  if (isMalloc(Op)) {
> +    if (CallInst* CI = extractMallocCallFromBitCast(Op)) {
> +      if (Op->hasOneUse() && CI->hasOneUse()) {
> +        EraseInstFromFunction(FI);
> +        EraseInstFromFunction(*CI);
> +        return EraseInstFromFunction(*cast<Instruction>(Op));
> +      }
> +    } else {
> +      // Op is a call to malloc
> +      if (Op->hasOneUse()) {
> +        EraseInstFromFunction(FI);
> +        return EraseInstFromFunction(*cast<Instruction>(Op));
> +      }
> +    }
> +  }

Are you planning top eliminate FreeInst someday?

> +++ llvm/trunk/lib/Transforms/Scalar/Reassociate.cpp Fri Sep 18  
> 17:35:49 2009
> @@ -121,7 +122,7 @@
>   if (I->getOpcode() == Instruction::PHI ||
>       I->getOpcode() == Instruction::Alloca ||
>       I->getOpcode() == Instruction::Load ||
> -      I->getOpcode() == Instruction::Malloc ||
> +      I->getOpcode() == Instruction::Malloc || isMalloc(I) ||
>       I->getOpcode() == Instruction::Invoke ||
>       (I->getOpcode() == Instruction::Call &&
>        !isa<DbgInfoIntrinsic>(I)) ||

Call is handled right below, I removed this.

> +++ llvm/trunk/lib/Transforms/Scalar/SCCP.cpp Fri Sep 18 17:35:49 2009
> @@ -400,7 +401,12 @@
>   void visitStoreInst     (Instruction &I);
>   void visitLoadInst      (LoadInst &I);
>   void visitGetElementPtrInst(GetElementPtrInst &I);
> -  void visitCallInst      (CallInst &I) { visitCallSite 
> (CallSite::get(&I)); }
> +  void visitCallInst      (CallInst &I) {
> +    if (isMalloc(&I))
> +      markOverdefined(&I);
> +    else
> +      visitCallSite(CallSite::get(&I));
> +  }

Calls to unknown functions are already marked overdefined, I removed  
this.

> +++ llvm/trunk/lib/Transforms/Scalar/TailDuplication.cpp Fri Sep 18  
> 17:35:49 2009
> @@ -129,7 +130,7 @@
>     if (isa<CallInst>(I) || isa<InvokeInst>(I)) return false;
>
>     // Allso alloca and malloc.
> -    if (isa<AllocationInst>(I)) return false;
> +    if (isa<AllocationInst>(I) || isMalloc(I)) return false;

This wasn't needed because of the line above it in the diff, I removed  
it.

> +++ llvm/trunk/lib/Transforms/Utils/InlineCost.cpp Fri Sep 18  
> 17:35:49 2009
> @@ -51,7 +52,7 @@
>       // Unfortunately, we don't know the pointer that may get  
> propagated here,
>       // so we can't make this decision.
>       if (Inst.mayReadFromMemory() || Inst.mayHaveSideEffects() ||
> -          isa<AllocationInst>(Inst))
> +          isa<AllocationInst>(Inst) || isMalloc(&Inst))
>         continue;

Calls were already handled above, so I removed this.

-Chris



More information about the llvm-commits mailing list