[llvm-commits] [llvm] r55674 - in /llvm/trunk: lib/Transforms/Scalar/LoopStrengthReduce.cpp test/Transforms/LoopStrengthReduce/2008-09-02-IVType.ll

[Devang Patel]

On Sep 2, 2008, at 4:07 PM, Dan Gohman wrote:

>
> On Sep 2, 2008, at 3:18 PM, Devang Patel wrote:
>
>> Author: dpatel
>> Date: Tue Sep  2 17:18:08 2008
>> New Revision: 55674
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=55674&view=rev
>> Log:
>> If all IV uses are extending integer IV then change the type of IV
>> itself, if possible.
>
> Hi Devang,
>
> Wouldn't it be better to do this transformation in IndVarSimplify,
> rather than in LoopStrengthReduce? It seems that this transformation
> simplifies loops, and makes it possible to compute trip counts in
> more cases, which would be beneficial to many passes.

Good idea. I was toying with the idea to make a separate little pass,  
but IndVarSimplify is a good fit.

> This code doesn't appear to handle integer wraparound correctly. At
> least, I didn't see anything that checked the trip count, or even
> the induction variable comparison. Am I missing something?

Nope. During restructuring, I accidently lost the check.

>> +    for (Value::use_iterator UI = PHI->use_begin(),
>> +           UE = PHI->use_end(); UI != UE; ++UI) {
>> +      Instruction *Use = dyn_cast<Instruction>(*UI);
>> +      if (!Use) {
>> +        TransformPhi = false;
>> +        break;
>> +      }
>
> The users of an instruction are always instructions, so you can
> use cast instead of dyn_cast here, and skip the if.
>
>> +  for (Value::use_iterator IncUI = Incr->use_begin(),
>> +         IncUE = Incr->use_end(); IncUI != IncUE; ++IncUI) {
>> +
>> +    Instruction *U2 = dyn_cast<Instruction>(*IncUI);
>
> Ditto.

ok

>
>
>> +  ConstantInt *C = NULL;
>> +  if (Incr->getOperand(0) == PHI)
>> +    C = dyn_cast<ConstantInt>(Incr->getOperand(1));
>> +  else if (Incr->getOperand(1) == PHI)
>> +    C = dyn_cast<ConstantInt>(Incr->getOperand(0));
>
> This code is essentially doing add-hoc scalar-evolution analysis, and
> at a brief readthrough, I didn't find code to verify that it's doing
> so correctly. In the code above, it doesn't verify that the add's user
> is in fact the same PHI that it's using. LSR and IndVars are both
> already ScalarEvolution clients. Could you replace this ad-hoc
> analysis by using the real ScalarEvolution analysis here?

ok :)

>> +    if (!TransformPhi || Incr == false || PHIUses.empty())
>> +      continue;
>
> Incr is a BinaryOperator*, so it shouldn't be compared with false.

Yup.

I'll fix these issues.
Thanks,
-
Devang