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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm 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
----------------
foad wrote:
> critson wrote:
> > @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.
> I am not familiar with this code.
> 
> Could you pass an optional `SlotIndexes *` into `updateTerminator`, and have it do the updates itself?
I'd expect these updates to happen at the time the actual mutation happens, so have updateTerminator doing t


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158144