Page MenuHomePhabricator

[MachineVerifier] Confirm that both ends of a tied def/use are live together
AbandonedPublic

Authored by reames on Feb 19 2021, 7:54 AM.

Details

Summary

If we have a tied def, the tied use should also be live in the same live interval. The exception is that the use may be undef, in which case the def is dead, but we don't appear to enforce that tightly. For the moment, check only the non-undef case.

The need to not check physical registers here bothers me. I added this because without it, test/CodeGen/X86/segmented-stacks-dynamic.ll fails. However, I can't claim to understand why it's necessary.

From reading the existing code, I believe this is the intended invariant. I am not overly familiar with this code (or at least I wasn't until recently), so I would appreciate a careful review.

Diff Detail

Unit TestsFailed

TimeTest
90 msx64 windows > LLVM.Instrumentation/InstrProfiling::profiling.ll
Script: -- : 'RUN: at line 1'; c:\ws\w16c2-1\llvm-project\premerge-checks\build\bin\opt.exe < C:\ws\w16c2-1\llvm-project\premerge-checks\llvm\test\Instrumentation\InstrProfiling\profiling.ll -mtriple=x86_64 -passes=instrprof -S | c:\ws\w16c2-1\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w16c2-1\llvm-project\premerge-checks\llvm\test\Instrumentation\InstrProfiling\profiling.ll --check-prefixes=CHECK,ELF,ELF_GENERIC

Event Timeline

reames created this revision.Feb 19 2021, 7:54 AM
reames requested review of this revision.Feb 19 2021, 7:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 19 2021, 7:54 AM
Herald added a subscriber: wdng. · View Herald Transcript

Realized I'd only run the X86 tests. Went and ran all targets, and hit some failures on AMDGPU and SystemZ.

Failed Tests (8):

LLVM :: CodeGen/AMDGPU/GlobalISel/insertelement.ll
LLVM :: CodeGen/AMDGPU/indirect-addressing-si.ll
LLVM :: CodeGen/AMDGPU/insert_vector_dynelt.ll
LLVM :: CodeGen/AMDGPU/llvm.amdgcn.image.dim.ll
LLVM :: CodeGen/AMDGPU/r600.global_atomics.ll
LLVM :: CodeGen/SystemZ/asm-18.ll
LLVM :: CodeGen/SystemZ/fold-memory-op-impl.ll
LLVM :: CodeGen/SystemZ/int-cmp-44.ll

I think I'm going to need some input from someone knowledgeable of these backends. As an example, the SystemZ/int-cmp-44.ll failure looks like this:

    • Bad machine code: tied def w/o use in same live range ***
  • function: f19
  • basic block: %bb.0 entry (0x560f21608b18) [0B;112B)
  • instruction: 48B %0:gr64bit = RISBG $noreg(tied-def 0), %2:gr64bit, 0, 190, 0, implicit-def dead $cc
  • liverange: [48r,160r:0) 0@48r
  • v. register: %0
  • ValNo: 0 (def 48r)

That looks like an invalid use of tied defs to me, but is there something I'm missing here?

Testcase? I also think this won't handle subregister indexes correctly. How does the x86 test fail?

llvm/lib/CodeGen/MachineVerifier.cpp
2732

I'm surprised this isn't checking readsReg instead of isDef

Realized I'd only run the X86 tests. Went and ran all targets, and hit some failures on AMDGPU and SystemZ.

Failed Tests (8):

LLVM :: CodeGen/AMDGPU/GlobalISel/insertelement.ll
LLVM :: CodeGen/AMDGPU/indirect-addressing-si.ll
LLVM :: CodeGen/AMDGPU/insert_vector_dynelt.ll
LLVM :: CodeGen/AMDGPU/llvm.amdgcn.image.dim.ll
LLVM :: CodeGen/AMDGPU/r600.global_atomics.ll
LLVM :: CodeGen/SystemZ/asm-18.ll
LLVM :: CodeGen/SystemZ/fold-memory-op-impl.ll
LLVM :: CodeGen/SystemZ/int-cmp-44.ll

I took a look into another failure CodeGen/AMDGPU/r600.global_atomics.ll
After Register Coalescing added verification check complains about subrange.

********** MACHINEINSTRS **********
# Machine code for function atomic_add_i32_offset: NoPHIs, TracksLiveness, TiedOpsRewritten

0B	bb.0.entry:
16B	  undef %4.sub0:r600_reg128 = MOV 1, 0, 0, 0, $alu_const, 0, 0, 0, 2058, 1, $pred_sel_off, 0, 0
32B	  %1:r600_treg32_x = ADD_INT 0, 0, 1, 0, 0, 0, $alu_const, 0, 0, 0, 2057, $alu_literal_x, 0, 0, 0, -1, 1, $pred_sel_off, 16, 0
80B	  dead %4:r600_reg128 = RAT_ATOMIC_ADD_NORET %4:r600_reg128(tied-def 0), %1:r600_treg32_x
96B	  RETURN

# End machine code for function atomic_add_i32_offset.

*** Bad machine code: tied def w/o use in same live range ***
- function:    atomic_add_i32_offset
- basic block: %bb.0 entry (0x1bcdbd8) [0B;112B)
- instruction: 80B	dead %4:r600_reg128 = RAT_ATOMIC_ADD_NORET %4:r600_reg128(tied-def 0), %1:r600_treg32_x
- liverange:   [80r,80d:1)  0@x 1@80r
- v. register: %4
- lanemask:    000000000000000E
- ValNo:       1 (def 80r)

Here,
16B undef %4.sub0:r600_reg128 = MOV 1, 0, 0, 0, $alu_const, 0, 0, 0, 2058, 1, $pred_sel_off, 0, 0
only subrange is defined.

mtrofin added inline comments.Feb 20 2021, 9:59 PM
llvm/lib/CodeGen/MachineVerifier.cpp
2749

Nit: you can just use Reg.isVirtual().

Testcase?

Not sure how to honestly. The whole idea is that an invalid live range should be impossible to construct, but that we want to catch it in verification if the register allocator does so.

I also think this won't handle subregister indexes correctly. How does the x86 test fail?

Can you expand on this a bit? I'm not sure what you mean by handling subregisters. What are the expectations for tied subregisters?

Testcase?

Not sure how to honestly. The whole idea is that an invalid live range should be impossible to construct, but that we want to catch it in verification if the register allocator does so.

We have unit tests in unittests/MI/LiveIntervalTest

I also think this won't handle subregister indexes correctly. How does the x86 test fail?

Can you expand on this a bit? I'm not sure what you mean by handling subregisters. What are the expectations for tied subregisters?

I believe the tied operands should have the same subregister index (although I'm not sure this is enforced). The same rules should apply, just on the subrange

reames updated this revision to Diff 327929.Mar 3 2021, 2:42 PM

Second attempt. Finally wrapped my head around the weird failures - they turned out to mostly be sub-register liveness cases. I'm definitely not clear on what the expected invariants are there, and intend to punt that part to someone who does in the future.

reames added a comment.Mar 3 2021, 2:45 PM

Testcase?

Not sure how to honestly. The whole idea is that an invalid live range should be impossible to construct, but that we want to catch it in verification if the register allocator does so.

We have unit tests in unittests/MI/LiveIntervalTest

I might be missing the obvious, but I don't see anything here to guide me on how to construct an intentionally malformed live interval and check for a verifier failure.

arsenm added a comment.Mar 3 2021, 6:05 PM

Testcase?

Not sure how to honestly. The whole idea is that an invalid live range should be impossible to construct, but that we want to catch it in verification if the register allocator does so.

We have unit tests in unittests/MI/LiveIntervalTest

I might be missing the obvious, but I don't see anything here to guide me on how to construct an intentionally malformed live interval and check for a verifier failure.

Well there's not really much to guide creating properly constructed intervals either...

Theoretically you could just do something like LIS->getInterval(Reg).removeSegment(Start, WrongEnd)

llvm/lib/CodeGen/MachineVerifier.cpp
2749

Maybe should have a FIXME to handle physical registers too

2754

I don't see why being implicit would be a special case for this. Implicit operands still need to follow all of the liveness rules

2757

I think checking isUndef is usually wrong, since undef on a subregister def still reads the register. This should probably be .readsReg()

arsenm requested changes to this revision.Mar 30 2021, 3:25 PM

Implicit operands aren't special. I also think the subreg handling is off

This revision now requires changes to proceed.Mar 30 2021, 3:25 PM
reames abandoned this revision.Fri, Apr 16, 1:49 PM

Abandoning change. I still think this is valuable, but I think it's also clear I don't have the necessary background knowledge to drive this. If anyone else wants to pick this up, feel free.