[llvm-commits] [llvm] r44687 - in /llvm/trunk: include/llvm/CodeGen/Passes.h lib/CodeGen/LLVMTargetMachine.cpp lib/CodeGen/MachineLICM.cpp lib/Target/PowerPC/PPCInstrInfo.td

[Chris Lattner]

>> +    // Visit all of the instructions of the loop. We want to visit
>> the subloops
>> +    // first, though, so that we can hoist their invariants first
>> into their
>> +    // containing loop before we process that loop.
>> +    SmallVector<MachineLoop*, 16> Loops;
>> +    GatherAllLoops(L, Loops);
>
> Seems to me this can be smarter. When you are gathering the loops, put
> loops of greater depth in the front of the queue then HoistRegion
> won't have to check if the BB is in the current loop level?

This should do a simple postorder traversal of the loop nest, there  
is no need to make a vector of the loops.  This ensures you visit  
inner loops before outer loops etc.

>> +    // If the loop contains the definition of an operand, then the
>> instruction
>> +    // isn't loop invariant.
>> +    if (CurLoop->contains(VRegDefs[Reg]->getParent()))
>> +      return false;
>
> Hmmm. I am not sure about this. What if the definition is in the
> preheader? Does contains() returns true? Chris, Owen?

a preheader is outside the loop.  The header is the "top" of the loop.

>> +    // FIXME: We are assuming at first that the basic blocks coming
>> into this
>> +    // loop have only one successor each. This isn't the case in
>> general because
>> +    // we haven't broken critical edges or added preheaders.
>> +    if (MBB->succ_size() != 1) return false;
>> +    assert(*MBB->succ_begin() == CurLoop->getHeader() &&
>> +           "The predecessor doesn't feed directly into the loop
>> header!");
>> +  }
>
> Are we going to create a preheader to hoist LICM to? Then we won't
> need to do all these checking? I am entirely sure about it. Someone
> please chime in.

Step #0 is to only work with simple loop forms.  Going forward, bill  
will generalize this to handle more cases.

>>
>> +
>> +  // Now move the instructions to the predecessors.
>> +  for (std::vector<MachineBasicBlock*>::iterator
>> +         I = Preds.begin(), E = Preds.end(); I != E; ++I)
>> +    MoveInstToBlock(*I, MI.clone());
>
> This will create multiple copies, it seems bad. Again, this should be
> fixable with a loop preheader?

Furthermore, this won't work, you'd have to insert phi nodes etc.   
You really really really don't want to do this bill.  Only handle  
loops that have a header with one predecessor that is outside the  
loop plz,

Very nice bill!

-Chris