[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 14:49:02 PDT 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