[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 wrote:
> 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
Simply passing SlotIndexes to updateTerminator is insufficient.
The actual instruction manipulation is performed by TargetInstrInfo::removeBranch so the instructions removed are target dependent.

To truly pass SlotIndexes all the way down is an all targets change.
I don't think it is unreasonable to pursue such a change, but it'll need to be a separate patch with many more reviewers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158144