This is an archive of the discontinued LLVM Phabricator instance.

Reland "[StackColoring] Remap PseudoSourceValue frame indices via MachineFunction::getPSVManager()""
ClosedPublic

Authored by MaskRay on Jan 27 2020, 3:17 PM.

Details

Diff Detail

Event Timeline

MaskRay created this revision.Jan 27 2020, 3:17 PM
MaskRay added a comment.EditedJan 27 2020, 3:36 PM

test/CodeGen/X86/catchpad-lifetime.ll (added in rL257158 @majnemer) appears to be bit-rotted. If I delete:

if (WinEHFuncInfo *EHInfo = MF->getWinEHFuncInfo())
  for (WinEHTryBlockMapEntry &TBME : EHInfo->TryBlockMap)
    for (WinEHHandlerType &H : TBME.HandlerArray)
      if (H.CatchObj.FrameIndex != std::numeric_limits<int>::max() &&
          SlotRemap.count(H.CatchObj.FrameIndex))
        H.CatchObj.FrameIndex = SlotRemap[H.CatchObj.FrameIndex];

Not test will break.

Unit tests: pass. 62248 tests passed, 0 failed and 816 were skipped.

clang-tidy: pass.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

rnk added a comment.Jan 27 2020, 3:45 PM

test/CodeGen/X86/catchpad-lifetime.ll (added in rL257158 @majnemer) appears to be bit-rotted. If I delete:
...
Not test will break.

I think it used to be covered by x64 tests, but then not long after in rG1ef654024f490b403494265c968570ca6bd800f3 we made most x64 catch objects fixed stack objects, which stack coloring should ignore. I ported one test to x86 in rGc7feb6b36aa8dac3cd20ffb9ad5980693ea9916e, but I'm not sure that one actually exercises this code.

rnk accepted this revision.Jan 27 2020, 3:49 PM

lgtm

llvm/lib/CodeGen/StackColoring.cpp
1080

Are you sure it wouldn't be better to do *SlotRemap.find(E.index())? Then it is clear that the operation is readonly. As is, I agree that it is impossible for this to insert a value, the vector for this index will only be non-empty if there is already an entry in the table.

This revision is now accepted and ready to land.Jan 27 2020, 3:49 PM
MaskRay updated this revision to Diff 240708.Jan 27 2020, 3:53 PM

Replace operator[] with

MF->getPSVManager().getFixedStack(SlotRemap.find(E.index())->second);

Unit tests: pass. 62248 tests passed, 0 failed and 816 were skipped.

clang-tidy: pass.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

This revision was automatically updated to reflect the committed changes.