[RewriteStatepointsForGC] Removes some unnecessary calls for operator[].

[staronj]

Summary

Working on clang-tidy check that is finding modifications of container you are iterating I found this lines of code. Here, we are using operator[] on std::map. In this case is not a problem, because we use the key of actual entry and it will not break anything, but we can easily remove operator[] by binding for-loop variable by reference.

[reames]

Can you give a bit of context on the motivation here?

[reames]

All of the const changes can be separate and submitted. (Please do so before posting revised patch to simplify review.)

lib/Transforms/Scalar/RewriteStatepointsForGC.cpp

762

Sink variable to use please.

Also, assigning to reference is mildly confusing. Can you add a comment to clarify?

819

This appears to be changing behaviour. The State variable previous survive to end of scope unmodified and is now modified part way through before hitting more conditional code. Please revert.

lib/Transforms/Scalar/RewriteStatepointsForGC.cpp

while (!Worklist.empty()) {
       else {
         llvm_unreachable("Unimplemented instruction case");
       }
     }
   }
 
 #ifndef NDEBUG
   DEBUG(dbgs() << "States after initialization:\n");
-  for (auto Pair : States) {
+  for (const auto &Pair : States) {
     DEBUG(dbgs() << " " << Pair.second << " for " << *Pair.first << "\n");
   }
 #endif
 
   // Return a phi state for a base defining value.  We'll generate a new
   // base state for known bases and expect to find a cached state otherwise.
   auto getStateForBDV = [&](Value *baseValue) {
     if (isKnownBaseResult(baseValue))
       return BDVState(baseValue);
     auto I = States.find(baseValue);
     assert(I != States.end() && "lookup failed!");
     return I->second;
   };
 
   bool Progress = true;
   while (Progress) {
 #ifndef NDEBUG
     const size_t OldSize = States.size();
 #endif
     Progress = false;
     // We're only changing values in this loop, thus safe to keep iterators.
     // Since this is computing a fixed point, the order of visit does not
     // effect the result.  TODO: We could use a worklist here and make this run
     // much faster.
-    for (auto Pair : States) {
+    for (auto &Pair : States) {
       Value *BDV = Pair.first;
+      BDVState &State = Pair.second;
       assert(!isKnownBaseResult(BDV) && "why did it get added?");
 
       // Given an input value for the current instruction, return a BDVState
       // instance which represents the BDV of that value.
       auto getStateForInput = [&](Value *V) mutable {
         Value *BDV = findBaseOrBDV(V, Cache);
         return getStateForBDV(BDV);
       };
for (auto &Pair : States) {
       } else {
         // The only instance this does not return a Conflict is when both the
         // vector operands are the same vector.
         auto *SV = cast<ShuffleVectorInst>(BDV);
         NewState = meetBDVState(NewState, getStateForInput(SV->getOperand(0)));
         NewState = meetBDVState(NewState, getStateForInput(SV->getOperand(1)));
       }
 
-      BDVState OldState = States[BDV];
-      if (OldState != NewState) {
+      if (State != NewState) {
         Progress = true;
-        States[BDV] = NewState;
+        State = NewState;
       }
     }
 
     assert(OldSize == States.size() &&
            "fixed point shouldn't be adding any new nodes to state");
   }
 
 #ifndef NDEBUG
   DEBUG(dbgs() << "States after meet iteration:\n");
-  for (auto Pair : States) {
+  for (const auto &Pair : States) {
     DEBUG(dbgs() << " " << Pair.second << " for " << *Pair.first << "\n");
   }
 #endif
 
   // Insert Phis for all conflicts
   // TODO: adjust naming patterns to avoid this order of iteration dependency
-  for (auto Pair : States) {
+  for (auto &Pair : States) {
     Instruction *I = cast<Instruction>(Pair.first);
-    BDVState State = Pair.second;
+    BDVState &State = Pair.second;
     assert(!isKnownBaseResult(I) && "why did it get added?");
     assert(!State.isUnknown() && "Optimistic algorithm didn't complete!");
 
     // extractelement instructions are a bit special in that we may need to
     // insert an extract even when we know an exact base for the instruction.
     // The problem is that we need to convert from a vector base to a scalar
     // base for the particular indice we're interested in.
     if (State.isBase() && isa<ExtractElementInst>(I) &&
         isa<VectorType>(State.getBaseValue()->getType())) {
       auto *EE = cast<ExtractElementInst>(I);
       // TODO: In many cases, the new instruction is just EE itself.  We should
       // exploit this, but can't do it here since it would break the invariant
       // about the BDV not being known to be a base.
       auto *BaseInst = ExtractElementInst::Create(
           State.getBaseValue(), EE->getIndexOperand(), "base_ee", EE);
       BaseInst->setMetadata("is_base_value", MDNode::get(I->getContext(), {}));
-      States[I] = BDVState(BDVState::Base, BaseInst);
+      State = BDVState(BDVState::Base, BaseInst);
     }
 
     // Since we're joining a vector and scalar base, they can never be the
     // same.  As a result, we should always see insert element having reached
     // the conflict state.
     assert(!isa<InsertElementInst>(I) || State.isConflict());
 
     if (!State.isConflict())
auto MakeBaseInstPlaceholder = [](Instruction *I) -> Instruction* {
         std::string Name = suffixed_name_or(I, ".base", "base_sv");
         return new ShuffleVectorInst(VecUndef, VecUndef, SV->getOperand(2),
                                      Name, SV);
       }
     };
     Instruction *BaseInst = MakeBaseInstPlaceholder(I);
     // Add metadata marking this as a base value
     BaseInst->setMetadata("is_base_value", MDNode::get(I->getContext(), {}));
-    States[I] = BDVState(BDVState::Conflict, BaseInst);
+    State = BDVState(BDVState::Conflict, BaseInst);
   }
 
   // Returns a instruction which produces the base pointer for a given
   // instruction.  The instruction is assumed to be an input to one of the BDVs
   // seen in the inference algorithm above.  As such, we must either already
   // know it's base defining value is a base, or have inserted a new
   // instruction to propagate the base of it's BDV and have entered that newly
   // introduced instruction into the state table.  In either case, we are
auto getBaseForInput = [&](Value *Input, Instruction *InsertPt) {
     if (Base->getType() != Input->getType() && InsertPt)
       Base = new BitCastInst(Base, Input->getType(), "cast", InsertPt);
     return Base;
   };
 
   // Fixup all the inputs of the new PHIs.  Visit order needs to be
   // deterministic and predictable because we're naming newly created
   // instructions.
-  for (auto Pair : States) {
+  for (const auto &Pair : States) {
     Instruction *BDV = cast<Instruction>(Pair.first);
     BDVState State = Pair.second;
 
     assert(!isKnownBaseResult(BDV) && "why did it get added?");
     assert(!State.isUnknown() && "Optimistic algorithm didn't complete!");
     if (!State.isConflict())
       continue;
 
#endif
       UpdateOperand(0); // vector operand
       UpdateOperand(1); // vector operand
     }
   }
 
   // Cache all of our results so we can cheaply reuse them
   // NOTE: This is actually two caches: one of the base defining value
   // relation and one of the base pointer relation!  FIXME
-  for (auto Pair : States) {
+  for (const auto &Pair : States) {
     auto *BDV = Pair.first;
     Value *Base = Pair.second.getBaseValue();
     assert(BDV && Base);
     assert(!isKnownBaseResult(BDV) && "why did it get added?");
 
     DEBUG(dbgs() << "Updating base value cache"
                  << " for: " << BDV->getName() << " from: "
                  << (Cache.count(BDV) ? Cache[BDV]->getName().str() : "none")