This is an archive of the discontinued LLVM Phabricator instance.

[PHIElimination] Handle subranges in LiveInterval updates

Authored by critson on Aug 16 2023, 10:33 PM.



Add handling for subrange updates in LiveInterval preservation.
This requires extending MachineBasicBlock::SplitCriticalEdge
to also update subrange intervals.

Diff Detail

Event Timeline

critson created this revision.Aug 16 2023, 10:33 PM
critson requested review of this revision.Aug 16 2023, 10:33 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 16 2023, 10:33 PM
arsenm accepted this revision.Aug 17 2023, 8:56 AM
arsenm added inline comments.

Probably should move to getOrCreateEmptyInterval helper

This revision is now accepted and ready to land.Aug 17 2023, 8:56 AM
critson updated this revision to Diff 551418.Aug 18 2023, 1:11 AM
  • Add getOrCreateEmptyInterval helper to LiveIntervals
This revision was landed with ongoing or failed builds.Sep 11 2023, 1:15 AM
This revision was automatically updated to reflect the committed changes.

There is a memory leak likely from this patch
Unfortunately build was broken by unrelated patch, not it's fixed, and we see the leak.

Please reply if you have a quick fix, or I will try to revert it.

vitalybuka reopened this revision.Sep 11 2023, 11:04 AM
This revision is now accepted and ready to land.Sep 11 2023, 11:04 AM

@vitalybuka - thanks for reverting, I will investigate.

critson updated this revision to Diff 556538.Sep 12 2023, 2:25 AM
  • Add additional fixes to SplitCriticalEdge
critson added inline comments.Sep 12 2023, 2:34 AM

@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.

foad added inline comments.Sep 12 2023, 2:39 AM

I am not familiar with this code.

Could you pass an optional SlotIndexes * into updateTerminator, and have it do the updates itself?

arsenm added inline comments.Sep 12 2023, 4:20 AM

I'd expect these updates to happen at the time the actual mutation happens, so have updateTerminator doing t

critson added inline comments.Sep 12 2023, 9:56 PM

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.

foad added inline comments.Sep 13 2023, 2:17 AM

Hmm, I think it's probably still the right way to do it though.

critson added inline comments.Sep 13 2023, 2:19 AM

I have put in a PR for fixing the SlotIndexes issue: