[PATCH] D35918: [GVNHoist] Factor out reachability to search for anticipable instructions quickly
Daniel Berlin via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Sep 6 22:15:07 PDT 2017
dberlin added a comment.
Sure, it's a function argument in a few other places.
The other places, you should be using auto :)
I marked a bunch of them.
In general, the only reason not to use auto is if the type is important and non-obvious.
In most of the cases, you would be better off naming variables better than exposing the types.
IE instead of CHIArg C, use auto CHIArg
(auto C is also fine if you think the type is obvious or unimportant)
================
Comment at: lib/Transforms/Scalar/GVNHoist.cpp:549
+ bool valueAnticipable(CHIIt Begin, CHIIt End, TerminatorInst *TI) const {
+ if (TI->getNumSuccessors() > std::distance(Begin, End))
+ return false; // Not enough args in this CHI.
----------------
Pass in a range instead of two iterators.
================
Comment at: lib/Transforms/Scalar/GVNHoist.cpp:552
+
+ for (CHIArg C : make_range(Begin, End)) {
+ BasicBlock *Dest = C.Dest;
----------------
auto C
================
Comment at: lib/Transforms/Scalar/GVNHoist.cpp:566
+ void checkSafety(CHIIt Begin, CHIIt End, BasicBlock *BB, InsKind K,
+ SmallVectorImpl<CHIArg> &Safe) {
int NumBBsOnAllPaths = MaxNumberOfBBSInPath;
----------------
Pass in a range instead?
================
Comment at: lib/Transforms/Scalar/GVNHoist.cpp:568
int NumBBsOnAllPaths = MaxNumberOfBBSInPath;
-
- SmallVecImplInsn::iterator II = InstructionsToHoist.begin();
- SmallVecImplInsn::iterator Start = II;
- Instruction *HoistPt = *II;
- BasicBlock *HoistBB = HoistPt->getParent();
- MemoryUseOrDef *UD;
- if (K != InsKind::Scalar)
- UD = MSSA->getMemoryAccess(HoistPt);
-
- for (++II; II != InstructionsToHoist.end(); ++II) {
- Instruction *Insn = *II;
- BasicBlock *BB = Insn->getParent();
- BasicBlock *NewHoistBB;
- Instruction *NewHoistPt;
-
- if (BB == HoistBB) { // Both are in the same Basic Block.
- NewHoistBB = HoistBB;
- NewHoistPt = firstInBB(Insn, HoistPt) ? Insn : HoistPt;
+ for (CHIIt B = Begin; B != End; ++B) {
+ Instruction *Insn = B->I;
----------------
range based for loop?
================
Comment at: lib/Transforms/Scalar/GVNHoist.cpp:609
+ // Pop the stack until Top(V) = Ve.
+ SmallVectorImpl<CHIArg> &VCHI = P->second;
+ for (SmallVectorImpl<CHIArg>::iterator It = VCHI.begin(), E = VCHI.end();
----------------
auto VCHI
================
Comment at: lib/Transforms/Scalar/GVNHoist.cpp:610
+ SmallVectorImpl<CHIArg> &VCHI = P->second;
+ for (SmallVectorImpl<CHIArg>::iterator It = VCHI.begin(), E = VCHI.end();
+ It != E;) {
----------------
auto It
================
Comment at: lib/Transforms/Scalar/GVNHoist.cpp:645
+ RenameStackType RenameStack;
+ for (auto it = df_begin(Root), E = df_end(Root); it != E; ++it) {
+ BasicBlock *BB = (*it)->getBlock();
----------------
depth_first instead of explicit iterator
================
Comment at: lib/Transforms/Scalar/GVNHoist.cpp:668
+ // accumulate hoistable candidates in HPL.
+ for (std::pair<BasicBlock *, SmallVector<CHIArg, 2>> &A : CHIBBs) {
+ BasicBlock *BB = A.first;
----------------
auto &A
================
Comment at: lib/Transforms/Scalar/GVNHoist.cpp:678
+ // [PreIt, PHIIt) form a range of CHIs which have identical VNs.
+ CHIIt PHIIt = std::find_if(CHIs.begin(), CHIs.end(),
+ [B](CHIArg &A) { return A != *B; });
----------------
auto
================
Comment at: lib/Transforms/Scalar/GVNHoist.cpp:680
+ [B](CHIArg &A) { return A != *B; });
+ CHIIt PrevIt = CHIs.begin();
+ while (PrevIt != PHIIt) {
----------------
auto
https://reviews.llvm.org/D35918
More information about the llvm-commits
mailing list