This is an archive of the discontinued LLVM Phabricator instance.

WIP: [MachineVerifier] Try harder to verify analyses
AbandonedPublic

Authored by foad on Jun 14 2022, 3:11 AM.

Details

Reviewers
None
Summary

Change MachineVerifier to call AnalysisUsage::addUsedIfAvailable for
analyses like LiveIntervals that it can verify.

Before this change, if pass P1 is the last user of analysis A1, the
legacy pass manager will free A1 after running P1 and *before* running
the machine verifier, so the analysis will not be verified even if P1
claimed to preserve it.

After this change, the machine verifier will be seen as a user of A1, so
it will not be freed until after the verifier has run and verified it.
The effect of this is that analyses are verified in more places than
they were before, which shows up some problems:

  • AMDGPU's SIOptimizeExecMaskingPreRA claimed to preserve all analyses but apparently does not preserve LiveIntervals
  • HexagonExpandCondsets claimed to preserve LiveIntervals but apparently does not
  • PPCTLSDynamicCall claimed to preserve LiveIntervals but apparently does not
  • Many (mostly X86) tests still fail because TwoAddressInstruction does not preserve LiveVariables

There are also some codegen differences, presumably because optional
analyses are no longer available in some places where they were
available before.

Diff Detail

Event Timeline

foad created this revision.Jun 14 2022, 3:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 14 2022, 3:11 AM
foad requested review of this revision.Jun 14 2022, 3:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 14 2022, 3:11 AM
arsenm added inline comments.Jun 14 2022, 5:53 AM
llvm/lib/Target/Hexagon/HexagonExpandCondsets.cpp
155

Probably should file an issue for this

llvm/lib/Target/PowerPC/PPCTLSDynamicCall.cpp
220

Ditto

lkail added a subscriber: lkail.Jun 17 2022, 4:45 AM
dfukalov added inline comments.
llvm/test/CodeGen/AMDGPU/cvt_f32_ubyte.ll
2285

I'm wondering the test started to use one more SReg for SI and VI after the patch.

2407

But it doesn't fix SRegs usage...

foad abandoned this revision.Jul 6 2022, 8:29 AM

I've split this into separate patches for separate analyses: D129200 D129201 D129208 D129213