[PATCH] D34025: [SCEV] Teach SCEVExpander to expand BinPow
Sanjoy Das via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Jun 16 21:04:16 PDT 2017
sanjoy accepted this revision.
sanjoy added a comment.
This revision is now accepted and ready to land.
lgtm with nits
================
Comment at: lib/Analysis/ScalarEvolutionExpander.cpp:767
+ while (E != OpsAndLoops.end() && *I == *E && Exponent != MaxExponent) {
+ ++Exponent; ++E;
+ }
----------------
Avoid braces here.
You could also write this loop using `find_if`.
Finally I'd perhaps suggest using a `uint64_t` for `Exponent` so that an overflow is practically impossible.
================
Comment at: lib/Analysis/ScalarEvolutionExpander.cpp:775
+ Value *Result = nullptr;
+ if (Exponent & 1u)
+ Result = P;
----------------
Can you get rid of this conditional and instead start `BinExp` from `1`?
================
Comment at: lib/Analysis/ScalarEvolutionExpander.cpp:792
// This is the first operand. Just expand it.
- Prod = expand(Op);
+ Prod = ExpandOpBinPowN();
} else if (Op->isAllOnesValue()) {
----------------
It is a bit hard to read `ExpandOpBinPowN()` and tell what all local state the call touches. Do you mind extracting out a static helper function so that the data flow is more obvious?
https://reviews.llvm.org/D34025
More information about the llvm-commits
mailing list