[PATCH] D126661: [MachineVerifier] Fix crash on early clobbered subreg operands.

Mon Jun 13 17:34:09 PDT 2022

arsenm added inline comments.

================
Comment at: llvm/test/MachineVerifier/verifier-ec-subreg-liveness.mir:1
+# RUN: llc -mtriple amdgcn-amd-amdhsa -run-pass=liveintervals,simple-register-coalescing -verify-machineinstrs -o - %s | FileCheck %s
+# REQUIRES: amdgpu-registered-target
----------------
dfukalov wrote:
> arsenm wrote:
> > dfukalov wrote:
> > > foad wrote:
> > > > arsenm wrote:
> > > > > dfukalov wrote:
> > > > > > arsenm wrote:
> > > > > > > tests in test/MachineVerifier should run none to just run the verifier. Shouldn't be running codegen passes
> > > > > > The crush is observed only when the verifier runs after a pass that has preserved liveintervals analysis.
> > > > > llc -march=amdgcn -run-pass liveintervals -verify-machineinstrs should work. Don't want to rely on a pass that could make changes for testing this
> > > > I have a vague recollection that `-run-pass liveintervals -verify-machineinstrs` isn't enough because the pass manager frees the liveintervals analysis as soon as it is no longer required, before the verifier has a chance to run. But I could be wrong.
> > > Yes, you're right, the verifier doesn't get liveinterval analysis in `-run-pass liveintervals -verify-machineinstrs` case. Regcoalescer seemed to me the smallest one from a few passes allowed to run unto crash.
> > I think I figured out some trick to get live intervals preserved here but I forget what it was. I think you also need to use -verify-coalescing to get a verifier run before the pass actually runs, instead of -verify-machineinstrs
> Actually in `-run-pass=liveintervals,simple-register-coalescing -verify-machineinstrs` case the verifier instance that runs into crash is performed after `liveintervals` pass and before coalescer. On the other hand `-run-pass=liveintervals,simple-register-coalescing -verify-coalescing` runs into verifier within coalescer (but before main work) so it seems a little more fragile.
I've been running into this problem a lot recently. I think we need to add a flag to force a dependency on the live intervals to MachineVerifier. We'll probably find a good number of failures in the test suite if it's forced on by default

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126661