[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/
Victor Hernandez
vhernandez at apple.com
Wed Oct 21 12:12:03 PDT 2009
Chris,
I fixed all your suggestions in rev 84772.
On Oct 17, 2009, at 10:24 PM, Chris Lattner wrote:
> 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.
Fixed:
// Update auto-upgraded malloc calls to "malloc".
// FIXME: Remove in LLVM 3.0.
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") {
Constant* RealMallocF = M->getFunction("malloc");
if (RealMallocF->getType() != MallocF->getType())
RealMallocF = ConstantExpr::getBitCast(RealMallocF, MallocF-
>getType());
MallocF->replaceAllUsesWith(RealMallocF);
MallocF->eraseFromParent();
MallocF = NULL;
}
}
>
>> + 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.
See above.
>
>> @@ -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'.
Fixed.
>
>> + 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.
Fixed.
>
>> + 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.
Fixed:
const Type *IntPtrTy = Type::getInt32Ty(Context);
const Type *Int8PtrTy = Type::getInt8PtrTy(Context);
>
>> + 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.
Fixed.
>
>> + // "autoupgrade_malloc" updated to "malloc" in
>> ValidateEndOfModule().
>> +
>> + Inst = cast<Instruction>(CallInst::CreateMalloc(BB, IntPtrTy,
>> Ty,
>> + Size, MallocF));
Fixed.
>
> CreateMalloc doesn't return something convertible to Instruction?
> Why do you need the cast<>?
Fixed.
>
>> +++ 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.
Fixed.
>
>> 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.
Fixed.
>
>> + 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.
It is not possible, copy-and-past error from other calls to
CallInst::CreateMalloc(). Deleted the IntegerCast.
>
>
>
>> + Value* Malloc = CallInst::CreateMalloc(CurBB, Int32Ty,
>> + Ty->getElementType(),
>> Size, NULL);
>> + I = cast<Instruction>(Malloc);
>
> again, why the cast?
Fixed.
>
>> +++ 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).
Thanks.
>
>> +++ 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.
Yup, that is next.
>
>> +++ 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.
Fixed.
>
>> +++ 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.
OK.
>
>>
>> =
>> =
>> =
>> =
>> =
>> =
>> =
>> =
>> =
>> =====================================================================
>> --- 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.
I fixed InstCombine so that the icmp is removed only if the malloc
call is removed (which requires explicit removal because the Worklist
won't DCE any calls since they can have side-effects). I also updated
the testcase to use FileCheck.
Victor
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20091021/2b980146/attachment.html>
More information about the llvm-commits
mailing list