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

Fri Jun 10 07:38:30 PDT 2022

arsenm added inline comments.

================
Comment at: llvm/lib/CodeGen/MachineVerifier.cpp:2433
+        if (SubRegIdx == 0)
+          checkLivenessAtDef(MO, MONum, DefIdx, *LI, Reg);

----------------
This does have a SubRangeCheck flag. Also wouldn't you want to perform the same checks over each subrange?

================
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:
> 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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126661