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

[Jay Foad via Phabricator via llvm-commits]

foad 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
----------------
critson wrote:
> 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.
Hmm, I think it's probably still the right way to do it though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158144