[llvm-commits] [llvm] r58625 - in /llvm/trunk: lib/Transforms/Scalar/IndVarSimplify.cpp test/Transforms/IndVarsSimplify/2008-11-03-Floating.ll

[Devang Patel]

On Nov 15, 2008, at 11:30 PM, Chris Lattner wrote:

>
> On Nov 3, 2008, at 10:32 AM, Devang Patel wrote:
>
>> Author: dpatel
>> Date: Mon Nov  3 12:32:19 2008
>> New Revision: 58625
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=58625&view=rev
>> Log:
>> Turn floating point IVs into integer IVs where possible.
>> This allows SCEV users to effectively calculate trip count.
>> LSR later on transforms back integer IVs to floating point IVs
>> later on to avoid int-to-float casts inside the loop.
>
> Very nice!  This does great things to simple loops.
>
>> @@ -466,6 +467,7 @@
>>  // auxillary induction variables.
>>  std::vector<std::pair<PHINode*, SCEVHandle> > IndVars;
>>
>> +  HandleFloatingPointIV(L);
>>  for (BasicBlock::iterator I = Header->begin(); isa<PHINode>(I); + 
>> +I) {
>>    PHINode *PN = cast<PHINode>(I);
>>    if (PN->getType()->isInteger()) { // FIXME: when we have fast- 
>> math, enable!
>
> This does an extra pass over all phi nodes in a loop.  Since there  
> can be many many of them, this is suboptimal.  Would it be  
> reasonable to make this happen at the same time when  
> EliminatePointerRecurrence is called?

yes. done.

>
>
>> +/// HandleFloatingPointIV - If the loop has floating induction  
>> variable
>> +/// then insert corresponding integer induction variable if  
>> possible.
>> +void IndVarSimplify::HandleFloatingPointIV(Loop *L) {
>
> Please add a comment giving an example of the sort of loop this  
> benefits.  Something like this is sufficient:
>
>  for (double i = 0; i < 10000; ++i)
>    bar((int)i);

done.

>
>
>>
>> +  BasicBlock *Header = L->getHeader();
>> +  SmallVector <PHINode *, 4> FPHIs;
>> +  Instruction *NonPHIInsn = NULL;
>> +
>> +  // Collect all floating point IVs first.
>> +  BasicBlock::iterator I = Header->begin();
>> +  while(true) {
>> +    if (!isa<PHINode>(I)) {
>> +      NonPHIInsn = I;
>> +      break;
>> +    }
>> +    PHINode *PH = cast<PHINode>(I);
>> +    if (PH->getType()->isFloatingPoint())
>> +      FPHIs.push_back(PH);
>> +    ++I;
>> +  }
>
> Why collect into a list and then handle all at once?  It seems that  
> you can just handle them on the fly like EliminatePointerRecurrence  
> does.

done.

>> +    FCmpInst *EC = dyn_cast<FCmpInst>(U1);
>> +    if (!EC)
>> +      EC = dyn_cast<FCmpInst>(U2);
>> +    if (!EC) continue;
>> +    bool skip = false;
>> +    Instruction *Terminator = EC->getParent()->getTerminator();
>> +    for(Value::use_iterator ECUI = EC->use_begin(), ECUE = EC- 
>> >use_end();
>> +        ECUI != ECUE; ++ECUI) {
>> +      Instruction *U = cast<Instruction>(ECUI);
>> +      if (U != Terminator) {
>> +        skip = true;
>> +        break;
>> +      }
>> +    }
>> +    if (skip) continue;
>
> Instead of using a 'skip' bool, please factor this inner loop out  
> into a static helper function.  However, why do you even do this at  
> all?  Why not just see if Terminator is a cond branch whose  
> condition is EC?

yup, why not :). Fixed.

I'll address rest of your comments in a separate commit.
Thanks!
-
Devang