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

[Dan Gohman]

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.

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?

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

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

>
> +    if (!TransformPhi || Incr == false || PHIUses.empty())
> +      continue;

Incr is a BinaryOperator*, so it shouldn't be compared with false.

Dan