[llvm-commits] [llvm] r84292 - in /llvm/trunk: examples/BrainF/ include/llvm/ lib/AsmParser/ lib/Bitcode/Reader/ lib/Transforms/IPO/ lib/Transforms/Scalar/ lib/VMCore/ test/Transforms/GlobalOpt/ test/Transforms/IndMemRem/ test/Transforms/InstCombine/
Chris Lattner
clattner at apple.com
Sat Oct 17 22:24:02 PDT 2009
On Oct 16, 2009, at 5:00 PM, Victor Hernandez wrote:
> URL: http://llvm.org/viewvc/llvm-project?rev=84292&view=rev
> Log:
> Autoupgrade malloc insts to malloc calls.
> Update testcases that rely on malloc insts being present.
>
> Also prematurely remove MallocInst handling from IndMemRemoval and
> RaiseAllocations to help pass tests in this incremental step.
Thanks for working on this Victor. I committed a few patches to do
random cleanups, here are some more things for you to look at:
> +++ llvm/trunk/lib/AsmParser/LLParser.cpp Fri Oct 16 19:00:19 2009
> @@ -69,6 +69,27 @@
> /// ValidateEndOfModule - Do final validity and sanity checks at the
> end of the
> /// module.
> bool LLParser::ValidateEndOfModule() {
> + // Update auto-upgraded malloc calls from "autoupgrade_malloc" to
> "malloc".
> + if (MallocF) {
> + MallocF->setName("malloc");
> + // If setName() does not set the name to "malloc", then there
> is already a
> + // declaration of "malloc". In that case, iterate over all
> calls to MallocF
> + // and get them to call the declared "malloc" instead.
> + if (MallocF->getName() != "malloc") {
> + Function* realMallocF = M->getFunction("malloc");
> + for (User::use_iterator UI = MallocF->use_begin(), UE=
> MallocF->use_end();
> + UI != UE; ) {
> + User* user = *UI;
> + UI++;
> + if (CallInst *Call = dyn_cast<CallInst>(user))
> + Call->setCalledFunction(realMallocF);
> + }
This should use V->replaceAllUsesWith() instead of manually walking
the use list. Doing this will handle non-call users as well as call
users. What happens if the prototypes don't agree for these two
functions? I think you'll get an abort. Please fix by using a
constantexpr bitcast to the expected type.
> + if (!realMallocF->doesNotAlias(0)) realMallocF-
> >setDoesNotAlias(0);
I don't think you have to care about this. We don't care that old
code be performant, just that it work.
> @@ -3466,10 +3487,21 @@
> if (Size && Size->getType() != Type::getInt32Ty(Context))
> return Error(SizeLoc, "element count must be i32");
>
> - if (Opc == Instruction::Malloc)
> - Inst = new MallocInst(Ty, Size, Alignment);
> - else
> + if (isAlloca)
> Inst = new AllocaInst(Ty, Size, Alignment);
Please use an early exit here to unnest the 'else'.
> + else {
> + // Autoupgrade old malloc instruction to malloc call.
Please add a FIXME to remove this in LLVM 3.0 like I added to other
places.
> + const Type* IntPtrTy = Type::getInt32Ty(Context);
> + const Type* Int8PtrTy = PointerType::getUnqual(Type::getInt8Ty
> (Context));
Please use 'const Type *IntPtrTy', watch the * placement. There is
now a helper function that you can use instead of
'PointerType::getUnqual(Type::getInt8Ty(Context));', please use it.
> + if (!MallocF)
> + // Prototype malloc as "void *autoupgrade_malloc(int32)".
> + MallocF = cast<Function>(M->getOrInsertFunction
> ("autoupgrade_malloc",
> + Int8PtrTy, IntPtrTy, NULL));
Why do you bother naming autoupgrade_malloc? This is in the user's
namespace, so it can conflict with a function named autoupgrade_malloc
in the user's code. Please just leave the name empty, making it
impossible for it to conflict.
> + // "autoupgrade_malloc" updated to "malloc" in
> ValidateEndOfModule().
> +
> + Inst = cast<Instruction>(CallInst::CreateMalloc(BB, IntPtrTy, Ty,
> + Size, MallocF));
CreateMalloc doesn't return something convertible to Instruction? Why
do you need the cast<>?
> +++ llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp Fri Oct 16
> 19:00:19 2009
> @@ -2044,14 +2044,21 @@
> }
>
> case bitc::FUNC_CODE_INST_MALLOC: { // MALLOC: [instty, op, align]
> + // Autoupgrade malloc instruction to malloc call.
FIXME remove in 3.0.
> if (Record.size() < 3)
> return Error("Invalid MALLOC record");
> const PointerType *Ty =
> dyn_cast_or_null<PointerType>(getTypeByID(Record[0]));
> Value *Size = getFnValueByID(Record[1], Type::getInt32Ty
> (Context));
> - unsigned Align = Record[2];
> if (!Ty || !Size) return Error("Invalid MALLOC record");
> - I = new MallocInst(Ty->getElementType(), Size, (1 << Align)
> >> 1);
> + if (!CurBB) return Error("Invalid malloc instruction with no
> BB");
> + const Type* Int32Ty = IntegerType::getInt32Ty(CurBB-
> >getContext());
* placement.
> + if (Size->getType() != Int32Ty)
> + Size = CastInst::CreateIntegerCast(Size, Int32Ty, false /
> *ZExt*/,
> + "", CurBB);
How is this possible? Size has to be int32 per the code above.
> + Value* Malloc = CallInst::CreateMalloc(CurBB, Int32Ty,
> + Ty->getElementType(),
> Size, NULL);
> + I = cast<Instruction>(Malloc);
again, why the cast?
> +++ llvm/trunk/lib/Transforms/IPO/IndMemRemoval.cpp Fri Oct 16
> 19:00:19 2009
> @@ -24,6 +24,7 @@
I removed this pass, it was specifically their to support malloc/free
instructions, it is not needed anymore (and not used in any case).
> +++ llvm/trunk/lib/Transforms/IPO/RaiseAllocations.cpp Fri Oct 16
> 19:00:19 2009
> @@ -1,4 +1,4 @@
> -//===- RaiseAllocations.cpp - Convert @malloc & @free calls to
> insts ------===//
> +//===- RaiseAllocations.cpp - Convert @free calls to insts ------
> ===//
Hopefully your future changes to remove FreeInst will completely zap
this pass.
> +++ llvm/trunk/lib/Transforms/Scalar/Reassociate.cpp Fri Oct 16
> 19:00:19 2009
> @@ -29,6 +29,7 @@
> #include "llvm/IntrinsicInst.h"
> #include "llvm/LLVMContext.h"
> #include "llvm/Pass.h"
> +#include "llvm/Analysis/MallocHelper.h"
> #include "llvm/Assembly/Writer.h"
> #include "llvm/Support/CFG.h"
> #include "llvm/Support/Debug.h"
> @@ -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)) ||
A check for isMalloc isn't needed here at all, because call is
handled. Please remove all the malloc logic and the #include.
> +++ llvm/trunk/test/Transforms/InstCombine/cast-malloc.ll Fri Oct 16
> 19:00:19 2009
> @@ -1,6 +1,6 @@
I removed this, it is now pointless. You convert it to test that
instcombine isn't doing anything.
>
> =
> =
> =
> =
> =
> =
> =
> =
> ======================================================================
> --- llvm/trunk/test/Transforms/InstCombine/malloc2.ll (original)
> +++ llvm/trunk/test/Transforms/InstCombine/malloc2.ll Fri Oct 16
> 19:00:19 2009
> @@ -1,5 +1,4 @@
> ; RUN: opt < %s -instcombine -S | grep {ret i32 0}
> -; RUN: opt < %s -instcombine -S | not grep malloc
> ; PR1313
>
> define i32 @test1(i32 %argc, i8* %argv, i8* %envp) {
This test is being miscompiled now. This is a serious bug that one of
your patches has introduced. We're now compiling (e.g.):
define i32 @test1(i32 %argc, i8* %argv, i8* %envp) {
%tmp15.i.i.i23 = malloc [2564 x i32] ; <[2564 x
i32]*> [#uses=1]
%c = icmp eq [2564 x i32]* %tmp15.i.i.i23,
null ; <i1>:0 [#uses=1]
%retval = zext i1 %c to i32 ; <i32> [#uses=1]
ret i32 %retval
}
into:
define i32 @test1(i32 %argc, i8* %argv, i8* %envp) {
%malloccall = tail call i8* @malloc(i32 ptrtoint ([2564 x i32]*
getelementptr ([2564 x i32]* null, i32 1) to i32)) ; <i8*> [#uses=0]
ret i32 0
}
The old code was correct because it would delete the call to malloc,
so it is safe to assume that the malloc (which never got run) never
ran out of space. The transformation that does this should just be
removed.
-Chris
More information about the llvm-commits
mailing list