Problems in removing a cloned instruction.

Hi all,
   I am trying to write a pass where i am creating a clone of a
function (the only difference being, the new function returns void ,
instead of any value).
I am creating a new Function Type with a void return type (rest being
the same as original function), and using this i am creating a new
function body. Then, i clone the body of the old function into new
function, but when ever i encounter a return instruction with a value,
i am creating a new instruction (return void) and removing the cloned
instruction.

The part of the code that makes these changes is below:

// This function, takes a basic block and clones it to another
identical basic block, the only changes occur for the return
instructions, which are replaced by ret void.

BasicBlock *ProgSlicer::CloneBasicBlock(const BasicBlock *BB,
                  DenseMap<const Value*, Value*> &ValueMap,
                  const char *NameSuffix, Function *F) {

        BasicBlock *NewBB = new BasicBlock("", F);
          if (BB->hasName()) NewBB->setName(BB->getName()+NameSuffix);

          bool hasCalls = false, hasDynamicAllocas = false,
hasStaticAllocas = false, isTerminal =false;

  // Loop over all instructions, and copy them over.
          for (BasicBlock::const_iterator II = BB->begin(), IE = BB->end();
                    II != IE; ++II)
          {
            const Instruction *NwInst = cast<Instruction>((II));

            Instruction *NewInst = II->clone();

            if(ReturnInst *RI = dyn_cast<ReturnInst>(NewInst))
            {
                cerr << "\n Return Found : " << *RI<< "\n";

                Instruction *NewRInst = new ReturnInst(); // creating
a new return instruction
                Value *v = NULL;

                if (II->hasName())
                    NewRInst->setName(II->getName()+NameSuffix);
                NewBB->getInstList().push_back(NewRInst);
                ValueMap[II] = NewRInst; // Add
instruction map to value.
                hasCalls |= isa<CallInst>(II);
            }

                if (II->hasName())
                NewInst->setName(II->getName()+NameSuffix);
                NewBB->getInstList().push_back(NewInst);
                ValueMap[II] = NewInst; // Add
instruction map to value.
                    hasCalls |= isa<CallInst>(II);

            if(ReturnInst *RI = dyn_cast<ReturnInst>(NewInst)) //
deleting the duplicate return
            {
                cerr << "\n INST : " << *NewInst << " Par : " <<
*(NewInst->getParent()) << "\n";
                NewInst->eraseFromParent();
            }

        }

    return NewBB;
      }

However,functionally pass seems to be working fine (using manual
printfs inside the code), but during the module verification pass, i
get an assert
"opt: Value.cpp:57: virtual llvm::Value::~Value(): Assertion
`use_empty() && "Uses remain when a value is destroyed!"' failed."

What could be the probem!!

Thanks alot.

Regards
Prabhat

Hi,

I'm gonna try to give some feedback, but I have only been working with LLVM
for a few days, so don't take what I'm saying without verifying :slight_smile:

BasicBlock *ProgSlicer::CloneBasicBlock(const BasicBlock *BB,
                  DenseMap<const Value*, Value*> &ValueMap,
                  const char *NameSuffix, Function *F) {

        BasicBlock *NewBB = new BasicBlock("", F);
          if (BB->hasName()) NewBB->setName(BB->getName()+NameSuffix);

          bool hasCalls = false, hasDynamicAllocas = false,
hasStaticAllocas = false, isTerminal =false;

  // Loop over all instructions, and copy them over.
          for (BasicBlock::const_iterator II = BB->begin(), IE = BB->end();
                    II != IE; ++II)
          {
            const Instruction *NwInst = cast<Instruction>((II));

What's this for? You don't seem to use it anywhere?

            Instruction *NewInst = II->clone();

Why do you clone the instruction here already? Can't you wait until you know
that II is not a ReturnInst? AFAIK you can simply dyn_cast II instead of
NewInst in the next line. This saves you from having to cleanup NewInst below
if you're not going to use it.

            if(ReturnInst *RI = dyn_cast<ReturnInst>(NewInst))
            {
                cerr << "\n Return Found : " << *RI<< "\n";

                Instruction *NewRInst = new ReturnInst(); // creating a new return instruction
                Value *v = NULL;

                if (II->hasName())
                    NewRInst->setName(II->getName()+NameSuffix);
                NewBB->getInstList().push_back(NewRInst);
                ValueMap[II] = NewRInst; // Add instruction map to value.
                hasCalls |= isa<CallInst>(II);

Can this one ever be true? Intuitively, if II is a ReturnInst (which is the
case inside this if), it can't be a CallInst as well?

            }

                if (II->hasName())
                NewInst->setName(II->getName()+NameSuffix);
                NewBB->getInstList().push_back(NewInst);
                ValueMap[II] = NewInst; // Add instruction map to value.
                    hasCalls |= isa<CallInst>(II);

Do you really need to do all this if II is a ReturnInst? Shouldn't this block
be in the else of the if(ReturnInst... above?

            if(ReturnInst *RI = dyn_cast<ReturnInst>(NewInst)) // deleting the duplicate return
            {
                cerr << "\n INST : " << *NewInst << " Par : " << *(NewInst->getParent()) << "\n";
                NewInst->eraseFromParent();

Is this really enough cleanup? AFAICS, this will at least present a memory
leak (since NewInst is a pointer, the cloned instruction will not have it's
destructor called automatically or something like that). I would suspect that
the cloned instruction is also sticking around with a some Value* (the one he
was planning to return) which triggers your error.

            }

        }

    return NewBB;
      }

Lastly, you're building a ValueMap, but not using it anywhere. Shouldn't you
be replacing any Value* used in the instructions using the ValueMap? Or is
that handled somewhere else (automatically perhaps?).

Gr.

Matthijs

Thanks Matthijs,
  Yes, you were right about the problem when i was deleting the return
instruction, the cloned use of that return instruction was still
there, which was giving the assert.
So, i did as you suggested, i do an early check if the instruction is
a return instruction, if yes, then i create a new (ret void)
instruction and add it to the basic block, if not then i clone the
instruction and do the rest of the thing as before.

It is working now nicely, by doing aforementioned changes.

Thanks again!!

Regards
Prabhat

PS : The new code is below:

BasicBlock *ProgSlicer::CloneBasicBlock(const BasicBlock *BB,
          DenseMap<const Value*, Value*> &ValueMap,
          const char *NameSuffix, Function *F) {
      
    BasicBlock *NewBB = new BasicBlock("", F);
      if (BB->hasName()) NewBB->setName(BB->getName()+NameSuffix);

      bool hasCalls = false, hasDynamicAllocas = false, hasStaticAllocas
= false, isTerminal =false;

  // Loop over all instructions, and copy them over.
      for (BasicBlock::const_iterator II = BB->begin(), IE = BB->end();
          II != IE; ++II)
      {
      
      const Instruction *NwInst = cast<Instruction>((II));
      
      // Don't Copy/Clone if the Return Instruction is found
      if(const ReturnInst *RI = dyn_cast<ReturnInst>(NwInst))
      {
        cerr << "\n Return Found : " << *RI<< "\n";
    
        Instruction *NewRInst = new ReturnInst(); //Create a new return
void instruction
        NewBB->getInstList().push_back(NewRInst); // Adding it to the basic bloc
      }
      
      else // otherwise
      {
        
        Instruction *NewInst = II->clone();
        if (II->hasName())
        NewInst->setName(II->getName()+NameSuffix);
        NewBB->getInstList().push_back(NewInst);
        ValueMap[II] = NewInst; // Add instruction map to value.
            hasCalls |= isa<CallInst>(II);
        
      }
    
      if (const AllocaInst *AI = dyn_cast<AllocaInst>(II)) {
        if (isa<ConstantInt>(AI->getArraySize()))
          hasStaticAllocas = true;
          else
          hasDynamicAllocas = true;
          }
      
    }

  return NewBB;
    }