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

[Matti Niemenmaa]

On 2010-05-20 23:20, Duncan Sands wrote:
>> +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.

Fair enough.

>> +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.

Whoops!

>> +  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?

Good catch, that's possible. It depends on how exactly 
Value::use_iterator works, which I don't know in enough detail. I did, 
however, find some code in Transforms/Scalar/SCCP.cpp which does an 
early increment to avoid such issues, so there probably is such a danger 
here.

Attached an amended patch with the cleanups and incrementing UI before 
the erases instead of after.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: instcombine-mallocs-only-null-compared-against.patch
Type: text/x-patch
Size: 6939 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20100521/f5b42c08/attachment.bin>