SIL: simplify deleting instructions while iterating over instructions.

[eeckstein]

On the C++ side we already supported this via the InstructionDeleter utility.
On the Swift side it was not possible to delete instructions during iteration at all.

This change adds support for Swift SIL and also significantly simplifies the C++ implementation.

Instead of the iterator-registration machinery (UpdatingListIterator and UpdatingInstructionIteratorRegistry) the new approach uses a simple trick to keep the iterators (C++ and Swift) in a consistent state: when "deleting" instructions, don't null the instruction's prev, next and parent pointers.
This allows to "jump" back into the not-deleted instruction list from a deleted instruction.

Beside the simpler implementation, it's also safer, because it doesn't require to delete instructions via a specific instance of an InstructionDeleter (which can be missed easily).

The new C++ APIs are SILBasicBlock::deletableInstructions() and SILBasicBlock::reverseDeletableInstructions().
In Swift, no new APIs are needed. One can just use the existing BasicBlock.instructions while being able to delete instructions in the list during iteration.

[eeckstein]

@swift-ci test

[eeckstein]

@swift-ci clean smoke test

[eeckstein]

@swift-ci test

[atrick]

This looks like a killer shortcut. I didn't know it was possible to do this and still reuse the ilist infrastructure.

Some changes in behavior though...

1. Dereferencing the iterator now gives you the deleted instruction instance. I guess that was intentional. I don't see any problem with it.

2. It's now possible to create an instruction iterator from a deleted instruction! I think that's dangerous. Can do something to prevent it?

3. The behavior of the new iterator is now incorrect when instructions are created. For example, a forward iterator at deleterInst will now skip over the newInst.

deletedInst
newInst
nextInst

I thought it was an important feature of the updating iterator to support instruction creation and deletion in the same code.

4. I think this breaks badly when an instruction moves to a different block and a deleted iterator happens to still refer to it.

I don't like registering iterators. But I'm not sure it's a good tradeoff if we create new classes of bugs instead.

[atrick]

Carry out the remaining deletion steps.

This comment doesn't seem to make sense any more. Maybe "now fully erase the instruction, which will harmlessly call willDeleteInstruction again"

(I'm not sure that's actually harmless but you're claiming it is)

[eeckstein]

IIUC, the existing UpdatingListIterator is meant to be updated when keeping it as a variable throughout a sequence of statements, e.g.

  auto iter = deleter.updatingRange(block).begin();
  ...
  block->erase(*iter);
  iter++;
  ...
  insertInstructionBefore(iter);
  ...
  do_something_with_instruction(*iter);

But we never use the updating iterator this way. We always use it in iterator loops, like

  for (SILInstruction *inst : deleter.updatingRange(&bb)) { ... }

(which is now for (SILInstruction &inst : bb.deletableInstructions()) { ... }).

It's now possible to create an instruction iterator from a deleted instruction! I think that's dangerous. Can do something to prevent it?

In such a for-loop it's never the case that an iterator of a deleted instruction is exposed. Unless you do something stupid like block->erase(&inst); auto iter = inst.getIterator(); (but that will also break with the existing UpdatingListIterator).

The behavior of the new iterator is now incorrect when instructions are created.

If you e.g. delete the current instruction and replace it with 2 new instructions:

  for (SILInstruction *inst : deleter.updatingRange(&bb)) {
    if (doSomeOptimization) {
      bb.erase(inst);
      builder.createInst1();
      builder.createInst2();
    }
  }

then with the old scheme, the next iteration after the current (deleted) instruction would see Inst2, which is kind of unintuitive: why skip just the first replacement instruction?
With the new iterator, the next iteration would see the original instruction which was located after the deleted instruction. And this makes sense: the next iteration visits the next instruction after the current instruction - or it's replacement(s).

I think this breaks badly when an instruction moves to a different block and a deleted iterator happens to still refer to it.

True, but it also breaks badly with the old mechanism. Though, we could catch this case with the new scheme (check against the end-iterator of the current instruction's parent block), I don't think it's worth the effort. It's a known limitation that instructions must not be moved to other blocks while iterating.
BTW, in Swift SIL, moving instruction is no problem, because we are always checking against the end-iterator of the current instruction's parent block.

[eeckstein]

@swift-ci smoke test

[eeckstein]

@swift-ci smoke test

[atrick]

My entire motivation for implementing the iterator registry was to handle inserting and moving instructions in all cases! You'll notice that the implementation was carefully designed to do that.

Obviously, if we're only deleting nodes from a single list, then the usual mechanism of keeping the old back-pointers works. I badly wanted to avoid the registry, because it's not as efficient, but I realized it was impossible to handle such cases correctly without it.

With the current registry, moving an instruction is always supposed to have the same iterator behavior as removing and reinserting the instruction. It's fine to move instructions into different blocks. Doing so should always update existing iterators so they continue referring to the same block. So, I'm not sure what you mean by "moving is broken".

With the current registry, all new instructions inserted after the current position should be visited. So this should visit both inst1 and inst2:

 for (SILInstruction *inst : deleter.updatingRange(&bb)) {
    if (doSomeOptimization) {
      bb.erase(inst);
      builder.createInst1();
      builder.createInst2();
    }
  }

The only way that you will skip a new instruction is if you advance the iterator before creating the instruction:

This explicitly skips the new instruction as expected:

auto iter = deleter.updatingRange(block).begin();
  ...
  block->erase(*iter);
  iter++;
  ...
  insertInstructionBefore(iter);

This does not skip the new instruction!

  auto iter = deleter.updatingRange(block).begin();
  auto next = std::next(iter)
  ...
  block->erase(*iter);
  ...
  insertInstructionBefore(next);

Again, I've fixed bugs in code that was doing this sort of thing. And that's the only reason that I thought the registry was needed.

Regarding the problem of having two instances of the deleter, I think that's a separate issue. The deleter should be a singleton pattern based on the current pass. Other than the small amount of overhead, I don't see any problem with registering the iterators.

The important thing is that you understand my original motivation. If you think keeping the old node pointers without updating them can't create subtle bugs then that's great. Or, if those problems can't occur in Swift sources (I don't understand how that works yet), then we could live dangerously in C++ in the meantime.

[eeckstein]

I'm not sure what you mean by "moving is broken".

For example, this crashes:

  for (SILInstruction *inst : deleter.updatingRange(bb)) {
    if (doSomeOptimization) {
      otherBlock->moveTo(otherPosition, inst);
    }
  }

(after the move the loop condition is comparing against the wrong end-iterator).

if those problems can't occur in Swift sources (I don't understand how that works yet)

In swift it's no problem because it's always comparing against the end iterator of the current instruction's block. This is e.g. happening in https://github.com/apple/swift/blob/main/SwiftCompilerSources/Sources/Optimizer/FunctionPasses/ObjCBridgingOptimization.swift where splitBlock moves instructions to another block while iterating over instructions.

With the current registry, all new instructions inserted after the current position should be visited.

That's only the case if you call deleter.getCallbacks().createdNewInst(newInst) explicitly (it's not done by the SILBuilder automatically)! If not, the behavior is the same as with the new approach. And IMO, it makes sense because why would an optimization pass want to see inserted instructions again? The common case is that passes want to visit "not yet handled" instructions and not artifacts from a previous peephole transformation.

we could live dangerously in C++ in the meantime.

It's definitely not a regression. And I think it's okay. We cannot catch all bad cases anyway and moving instructions to a different block is not a common thing to do.

Anyway, thanks a lot for taking the time to review and for the discussion!

[atrick]

for (SILInstruction *inst : deleter.updatingRange(bb)) {
if (doSomeOptimization) {
otherBlock->moveTo(otherPosition, inst);
}
}

Well, that's supposed to work just like erasing inst and inserting it in a different block. The iterator should always remain in the original block. Maybe someone changed the implementation of moveTo?

As I mentioned, I also designed the iterator registry to correctly handle moving within the block and inserting new instructions.

[atrick]

That's only the case if you call deleter.getCallbacks().createdNewInst(newInst) explicitly (it's not done by the SILBuilder automatically)! If not, the behavior is the same as with the new approach. And IMO, it makes sense because why would an optimization pass want to see inserted instructions again? The common case is that passes want to visit "not yet handled" instructions and not artifacts from a previous peephole transformation.

Not using the new instruction callback is definitely a bug. The fact that it's easy to mess that up is a separate issue.

Having an iterator that sometimes sees new instructions and other times mysteriously skips them does not make sense.

[eeckstein]

Well, that's supposed to work just like erasing inst and inserting it in a different block.

I don't know how this could have ever worked. There is (and never was) a callback for moving instructions - only for deleting instructions. So the iterator registry cannot be informed of moving instructions.

[atrick]

I don't know how this could have ever worked. There is (and never was) a callback for moving instructions - only for deleting instructions. So the iterator registry cannot be informed of moving instructions.

Ok, there are only two relevant questions then:

1. There were some loops (maybe SILCombine) that relied on visiting new instructions. Maybe that's handled via some other worklist so we just don't care about the iterator?

2. Should moving instructions be handled in the Swift sources, according to the original intention of updating iterators? If it's easy to intercept the removal of any instruction, then it could be made to work. On the other hand, it's a lot of overhead, and you mentioned that Swift can already avoid the dangerous problem of comparing against the wrong end iterator.

[eeckstein]

Maybe that's handled via some other worklist so we just don't care about the iterator?

Exactly. It's https://github.com/apple/swift/blob/0d666e291183466f4abd4b83484aa1293eb59f84/lib/SILOptimizer/SILCombiner/SILCombiner.h#L73

On the other hand, it's a lot of overhead, and you mentioned that Swift can already avoid the dangerous problem of comparing against the wrong end iterator.

Yeah, I think it's fine to not do any special handling for moving instructions. It's just important that it cannot result in a crash and the behavior is not undefined.