[PATCH] D91066: [SlotIndexes] Consider existing index range in insertMBBIntoMaps

[Jay Foad via Phabricator via llvm-commits]

foad added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/SlotIndexes.h:621
         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();
----------------
critson wrote:
> qcolombet wrote:
> > 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.
> 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.
I found the whole concept of InsertionPoint here confusing, so I removed it and simplified this code: D94311.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91066/new/

https://reviews.llvm.org/D91066