[llvm-commits] [PATCH] InstCombine: remove malloc+free if malloc's only uses are comparisons to null
Duncan Sands
baldrick at free.fr
Thu May 20 13:20:17 PDT 2010
Hi Matti,
> On 2010-05-17 14:07, Matti Niemenmaa wrote:
>> The code in InstCombineCompares.cpp makes me wonder whether we should be
>> operating on the malloc call to begin with? That is, move this 'malloc +
>> [icmps] + [free]' removal from visitFree and InstCombineCompares.cpp to
>> a new InstCombiner::visitMalloc function which would be called like
>> visitFree currently is, from InstCombiner::visitCallInst.
>
> I did this and the other mentioned changes: replacing the icmps directly
> instead of changing their operands and only working on icmp eq/ne
> instead of all kinds of icmp.
>
> Updated patch attached.
thanks for doing this. It looks good to me. Some small comments:
> +static bool IsOnlyNullComparedAndFreed(const Value &V) {
> + for (Value::const_use_iterator UI = V.use_begin(), UE = V.use_end();
> + UI != UE; ++UI) {
How about doing
if (isFreeCall(*UI))
continue;
already here, and ...
> + if (const ICmpInst *ICI = dyn_cast<ICmpInst>(*UI)) {
> + ICmpInst::Predicate Pred = ICI->getPredicate();
> + if (Pred == ICmpInst::ICMP_EQ || Pred == ICmpInst::ICMP_NE &&
> + (isa<ConstantPointerNull>(ICI->getOperand(0)) ||
> + isa<ConstantPointerNull>(ICI->getOperand(1))))
> + continue;
> + } else if (isFreeCall(*UI))
> + continue;
... removing this unbeautiful else.
> +Instruction *InstCombiner::visitMalloc(Instruction &MI) {
> + // If we have a malloc call which is only used in any amount of comparisons
> + // to null and free calls, delete the calls and replace the comparisons with
> + // true or false as appropriate.
> + CallInst *FreeCall = NULL;
Unused variable.
> + if (IsOnlyNullComparedAndFreed(MI)) {
> + for (Value::use_iterator UI = MI.use_begin(), UE = MI.use_end();
> + UI != UE; ++UI) {
> + // We can assume that every remaining use is a free call or an icmp eq/ne
> + // to null.
> + if (isFreeCall(*UI)) {
> + EraseInstFromFunction(*cast<CallInst>(*UI));
> + continue;
> + }
> + ICmpInst *I = cast<ICmpInst>(*UI);
> + ReplaceInstUsesWith(*I, ConstantInt::get(Type::getInt1Ty(I->getContext()),
> + I->isFalseWhenEqual()));
> + EraseInstFromFunction(*I);
> + }
This seems fine, except I have to wonder: first you erase *UI and then you
increment UI (the ++UI in the for loop). Maybe this means the increment is
accessing freed memory?
Ciao,
Duncan.
More information about the llvm-commits
mailing list