[PATCH] D158144: [PHIElimination] Handle subranges in LiveInterval updates

[Carl Ritson via Phabricator via llvm-commits]

critson added inline comments.


================
Comment at: llvm/lib/CodeGen/MachineBasicBlock.cpp:1169
 
-  // If updateTerminator() removes instructions, we need to remove them from
-  // SlotIndexes.
-  SmallVector<MachineInstr*, 4> Terminators;
-  if (Indexes) {
-    for (MachineInstr &MI :
-         llvm::make_range(getFirstInstrTerminator(), instr_end()))
-      Terminators.push_back(&MI);
-  }
+  if (!ChangedIndirectJump) {
+    // If updateTerminator() removes instructions they need to be removed from
----------------
@arsenm / @foad - can you let me know if you are happy with this fix to obviously wrong use after free code that existed previously?

Basically the old code takes pointers to terminators that will be removed by TII->removeBranch() then dereferences them later to update the maps.

Other possible ways I can see to this:
1. Provide a way to remove pointers from maps directly, without dereferencing (this is fairly trivial to implement).
2. Add a class variable which stores a pointer to SlotIndexes to update, and make erase() use this to call removeMachineInstrFromMaps().  Manipulate the new variable during SplitCriticalEdge to catch any erase() calls.

The problems are:
(1) implies SlotIndexes should be able to retain pointers to deleted data, which is a bad idea.
(2) adds (minor) overhead to every erase() call, just to support this edge case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158144