[PATCH] D10477: Expand loop reroll to handle loop with multiple induction variables and negative increment -part 1/3
James Molloy
james.molloy at arm.com
Wed Jul 15 00:38:59 PDT 2015
jmolloy requested changes to this revision.
jmolloy added a comment.
This revision now requires changes to proceed.
Hi,
Thanks for making these changes - it looks much better now. A few comments still.
Cheers,
James
================
Comment at: lib/Transforms/Scalar/LoopRerollPass.cpp:496
@@ +495,3 @@
+ continue;
+ IVToIncMap[I] = IncSCEV->getValue()->getZExtValue();
+ }
----------------
I think this line and line 503 can both be merged, and put on line 505. There's no need to ZExt this one and SExt the other - both can be SExted.
================
Comment at: lib/Transforms/Scalar/LoopRerollPass.cpp:499
@@ +498,3 @@
+ else if (AInt.isNegative()) {
+ const SCEVConstant *PIncSCEV =
+ dyn_cast<SCEVConstant>(SE->getNegativeSCEV(IncSCEV));
----------------
This can be much simpler. You can just use APInt::abs():
AInt = AInt.abs();
if (AInt.isZero() || AInt.uge(MaxInc))
continue;
IVToIncMap[I] = AInt.getSExtValue();
================
Comment at: lib/Transforms/Scalar/LoopRerollPass.cpp:677
@@ -661,2 +676,3 @@
- for (auto *UU : BO->users()) {
+ if (!BO && !isa<GetElementPtrInst>(U))
+ return false;
----------------
I'm not sure how adding GEP support here relates to negative values. Was this an accidentally added hunk?
================
Comment at: lib/Transforms/Scalar/LoopRerollPass.cpp:725
@@ -706,9 +724,3 @@
- // FIXME: Add support for negative values.
- if (V < 0) {
- DEBUG(dbgs() << "LRR: Aborting due to negative value: " << V << "\n");
- return false;
- }
-
- Roots[V] = cast<Instruction>(I);
+ Roots[std::abs(V)] = cast<Instruction>(I);
}
----------------
I think you need to use std::abs() above, on line 721 too. Otherwise the guard on line 721 is useless.
================
Comment at: lib/Transforms/Scalar/LoopRerollPass.cpp:1280
@@ -1270,1 +1279,3 @@
+ int64_t Inc = IVToIncMap[IV];
+ bool Negative = Inc < 0;;
const DataLayout &DL = Header->getModule()->getDataLayout();
----------------
Double semicolon (;;).
Also, you can squash this into just one statement:
bool Negative = IVToIncMap[IV] < 0;
Repository:
rL LLVM
http://reviews.llvm.org/D10477
More information about the llvm-commits
mailing list