[AccessMarker] Fix a reverse instruction iterator.

I reversed this loop's direction over the instruction list and forgot to change
the order of erasing an instruction with respect to advancing the iterator.

Thankfully ASAN is far smarter than I.

Converting between forward/reverse iterators makes the loop unreadable.
Add an iterator return value to BasicBlock::erase(SILInstruction*).

Author: atrick

include/swift/SIL/SILBasicBlock.h

@@ -94,7 +94,7 @@ public llvm::ilist_node<SILBasicBlock>, public SILAllocated<SILBasicBlock> {
   void push_back(SILInstruction *I);
   void push_front(SILInstruction *I);
   void remove(SILInstruction *I);
-  void erase(SILInstruction *I);
+  iterator erase(SILInstruction *I);
 
   SILInstruction &back() { return InstList.back(); }
   const SILInstruction &back() const {

lib/SIL/SILBasicBlock.cpp

@@ -90,12 +90,13 @@ void SILBasicBlock::remove(SILInstruction *I) {
   InstList.remove(I);
 }
 
-void SILBasicBlock::erase(SILInstruction *I) {
+SILBasicBlock::iterator SILBasicBlock::erase(SILInstruction *I) {
   // Notify the delete handlers that this instruction is going away.
   getModule().notifyDeleteHandlers(&*I);
   auto *F = getParent();
-  InstList.erase(I);
+  auto II = InstList.erase(I);
   F->getModule().deallocateInst(I);
+  return II;
 }
 
 /// This method unlinks 'self' from the containing SILFunction and deletes it.

lib/SILOptimizer/Mandatory/AccessMarkerElimination.cpp

@@ -45,17 +45,17 @@ struct AccessMarkerElimination : SILModuleTransform {
       // iterating in reverse eliminates more begin_access users before they
       // need to be replaced.
       for (auto &BB : reversed(F)) {
-        for (auto II = BB.rbegin(), IE = BB.rend(); II != IE;) {
-          SILInstruction *inst = &*II;
-          ++II;
+        // Don't cache the begin iterator since we're reverse iterating.
+        for (auto II = BB.end(); II != BB.begin();) {
+          SILInstruction *inst = &*(--II);
 
           if (auto beginAccess = dyn_cast<BeginAccessInst>(inst)) {
             beginAccess->replaceAllUsesWith(beginAccess->getSource());
-            beginAccess->eraseFromParent();
+            II = BB.erase(beginAccess);
           }
           if (auto endAccess = dyn_cast<EndAccessInst>(inst)) {
             assert(endAccess->use_empty());
-            endAccess->eraseFromParent();
+            II = BB.erase(endAccess);
           }
         }
       }