[PATCH] D35918: [GVNHoist] Factor out reachability to search for anticipable instructions quickly

[Daniel Berlin via Phabricator via llvm-commits]

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