[llvm-commits] [PATCH] InstCombine: remove malloc+free if malloc's only uses are comparisons to null

[Duncan Sands]

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.