[llvm-commits] [llvm] r55674 - in /llvm/trunk: lib/Transforms/Scalar/LoopStrengthReduce.cpp test/Transforms/LoopStrengthReduce/2008-09-02-IVType.ll
Devang Patel
dpatel at apple.com
Tue Sep 2 16:43:25 PDT 2008
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
More information about the llvm-commits
mailing list