This is an archive of the discontinued LLVM Phabricator instance.

[SlotIndexes] Consider existing index range in insertMBBIntoMaps
AbandonedPublic

Authored by critson on Nov 9 2020, 3:32 AM.

Details

Summary

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

Diff Detail

Event Timeline

critson created this revision.Nov 9 2020, 3:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 9 2020, 3:33 AM
critson requested review of this revision.Nov 9 2020, 3:33 AM
qcolombet added inline comments.Dec 3 2020, 10:03 AM
llvm/include/llvm/CodeGen/SlotIndexes.h
621

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 added inline comments.Dec 3 2020, 7:01 PM
llvm/include/llvm/CodeGen/SlotIndexes.h
621

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.

foad added a subscriber: foad.
foad added inline comments.
llvm/include/llvm/CodeGen/SlotIndexes.h
621

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

critson abandoned this revision.Jan 11 2021, 5:59 PM

Superseded by D94311.