[SlotIndexes] Consider existing index range in insertMBBIntoMaps

[critson]

Summary

When an insertion point is specified in insertMBBIntoMaps the
index range of existing instructions in the block needs to be
considered.

[critson]

Ping

[qcolombet]

llvm/include/llvm/CodeGen/SlotIndexes.h

621–625

I am not sure I understand the patch.
When InsertionPoint is set the block will be inserted before it, thus the endEntry for the block is the entry of the insertion point.

Admittedly, that behavior is weird (because that means the users of this method must make sure to split/join the blocks accordingly), but this is what the comment says!

Side-comment: Is that code reachable? A quick grep tells me that InsertionPoint is never passed around. I.e., we should just delete it.

[critson]

llvm/include/llvm/CodeGen/SlotIndexes.h

621–625

This fixes the case where an inserted block contains multiple instructions.
This is done by setting up the inserted block so that it encompasses the entire range of slot indexes until the next MBB in program order (rather than just the insertion point).
The renumbering of indexes will then occur correctly from the beginning of the program until the start of the following block, renumbering all instructions in the inserted block.
I think the variable names startEntry/endEntry in the existing code are potentially confusing, but I do not know what they should be named instead.
The behaviour mention in the comment is still accurate with this change.

InsertionPoint is used by MachineBasicBlock::splitAt, which I am trying to fix in D91064. Note D91064 contains tests for behaviour fixed by this patch. A couple of other changes are dependent on splitAt working correctly.

[critson]

Ping

[critson]

Rebase

[foad]

llvm/include/llvm/CodeGen/SlotIndexes.h

621–625

I found the whole concept of InsertionPoint here confusing, so I removed it and simplified this code: D94311.

[critson]

Superseded by D94311.

llvm/include/llvm/CodeGen/SlotIndexes.h

public:
     /// Add the given MachineBasicBlock into the maps.
     /// If \p InsertionPoint is specified then the block will be placed
     /// before the given machine instr, otherwise it will be placed
     /// before the next block in MachineFunction insertion order.
     void insertMBBInMaps(MachineBasicBlock *mbb,
                          MachineInstr *InsertionPoint = nullptr) {
       MachineFunction::iterator nextMBB =
         std::next(MachineFunction::iterator(mbb));
+      bool isLastMBB = nextMBB == mbb->getParent()->end();
 
       IndexListEntry *startEntry = nullptr;
       IndexListEntry *endEntry = nullptr;
       IndexList::iterator newItr;
       if (InsertionPoint) {
         startEntry = createEntry(nullptr, 0);
-        endEntry = getInstructionIndex(*InsertionPoint).listEntry();
-        newItr = indexList.insert(endEntry->getIterator(), startEntry);
-      } else if (nextMBB == mbb->getParent()->end()) {
+        endEntry = isLastMBB ? &indexList.back()
+                             : getMBBStartIdx(&*nextMBB).listEntry();
+        auto insIdx = getInstructionIndex(*InsertionPoint).listEntry();
+        newItr = indexList.insert(insIdx->getIterator(), startEntry);
+      } else if (isLastMBB) {
         startEntry = &indexList.back();
         endEntry = createEntry(nullptr, 0);
         newItr = indexList.insertAfter(startEntry->getIterator(), endEntry);
       } else {
         startEntry = createEntry(nullptr, 0);
         endEntry = getMBBStartIdx(&*nextMBB).listEntry();
         newItr = indexList.insert(endEntry->getIterator(), startEntry);
       }