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

[Jay Foad via Phabricator via llvm-commits]

foad added a reviewer: qcolombet.
foad added a subscriber: qcolombet.
foad added a comment.

I think this needs more explanation. It seems like it only fails verification when the same instruction has both a normal subreg def and an early-clobber subreg def of the same register. Is that correct? And the problem is that the super range starts at the early clobber slot (16e), but the subrange for the normal subreg def starts at the reg slot (16r):

  %0 [16e,32r:0) 0 at 16e  L0000000000000003 [16e,32r:0) 0 at 16e  L000000000000000C [16r,32r:0) 0 at 16r  weight:0.000000e+00
  ...
  *** Bad machine code: Inconsistent valno->def ***
  - function:    sub0
  - basic block: %bb.0  (0xbdc57a8) [0B;64B)
  - instruction: 16B	INLINEASM &"" [attdialect], $0:[regdef-ec:VGPR_32], def undef early-clobber %0.sub0:vreg_64, $1:[regdef:VGPR_32], def undef %0.sub1:vreg_64
  - operand 5:   undef %0.sub1:vreg_64
  - liverange:   [16e,32r:0) 0 at 16e
  - v. register: %0
  - ValNo:       0 (def 16e)
  - at:          16r

I think your fix looks OK but I'd also like to hear opinions from @MatzeB or @qcolombet.



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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126661