[RegisterCoalescer] Avoid erasing copy that breaks subranges

[tpr]

Summary

I found a case where erasing a copy in coalescing causes a subrange to
be modified incorrectly.

It looks like it would be quite a large change to fix this properly,
such that subranges are updated correctly when a subreg is dead in and
live out of a copy being erased.

A possible alternative would be to recalculate subranges from scratch
for a register that is affected by this problem.

For now, I have just disabled that case of erasing a copy when subranges
are present.

Change-Id: Ib85a4e7e5458ec1b156cb84207c51cab63e92635

[kparzysz]

What exactly is done wrong in the testcase without your change? I see one extra IMPLICIT_DEF in the output, compared to running it with your change.

Updating live ranges should work correctly, disabling erasing copies is not the right thing to do.

Could you run the input through -run-pass=none first? It will produce a slightly better-looking .mir.

[kparzysz]

I missed the comment in the .mir file.

[tpr]

V2: This is a revised fix. Still not the proper fix, but at least it
doesn't stop a copy being erased.

Also tidied the test as suggested.

Here's the new commit message, as I don't think phabricator updates the
summary box:

[RegisterCoalescer] Recalculate subranges in some cases

Summary:
I found a case where erasing a copy in coalescing causes a subrange to
be modified incorrectly.

It looks like it would be quite a large change to fix this properly,
such that subranges are updated correctly when a subreg is dead in and
live out of a copy being erased.

So for now, in the case causing my problem, it now just recalculates
the whole live interval after the coalesce.

[tpr]

To try and diagnose this kind of problem in the future, I added code (not in this change) to always recalculate the live interval if there are subranges, and compare it with the one updated in RegisterCoalescer.cpp. The problem I ran into then is that RegisterCoalescer remembers more information about when subregs are undef and thus considered not live than is available in the MIR after coalescing. I wonder if there is some way of recording undef subregs in a use in MIR such that recalculating a live interval should always give the same result as updating in coalescing, and if it doesn't then you've found a bug.

[kparzysz]

Could you try D41802 and see if it works for you? I works with the testcase and it passes my local tests.

[tpr]

Superseded by D48102 from Krzysztof.

lib/CodeGen/RegisterCoalescer.cpp

JoinVals::analyzeValue(unsigned ValNo, JoinVals &Other) {
   if (OtherLRQ.isKill() && OtherLRQ.endPoint() <= VNI->def)
     return CR_Keep;
 
   // Handle the case where VNI and OtherVNI can be proven to be identical:
   //
   //   %other = COPY %ext
   //   %this  = COPY %ext <-- Erase this copy
   //
-  if (DefMI->isFullCopy() && !CP.isPartial()
+  // Avoid this case when there are subregs, as it can result in an incorrect
+  // subreg live range.
+  //
+  if (!TrackSubRegLiveness && DefMI->isFullCopy() && !CP.isPartial()
       && valuesIdentical(VNI, V.OtherVNI, Other))
     return CR_Erase;
 
   // If the lanes written by this instruction were all undef in OtherVNI, it is
   // still safe to join the live ranges. This can't be done with a simple value
   // mapping, though - OtherVNI will map to multiple values:
   //
   //   1 %dst:ssub0 = FOO                <-- OtherVNI

test/CodeGen/AMDGPU/coalescing-with-subregs-in-loop-bug.mir

+# RUN: llc -mtriple=amdgcn--amdpal -mcpu=gfx803 -run-pass=simple-register-coalescing,rename-independent-subregs %s -o - | FileCheck -check-prefix=GCN %s
+
+# This test is for a bug where the following happens:
+#
+# Inside the loop, %8611.sub2 is used in a V_LSHLREV whose result is then used
+# in an LDS read. %8611 is a 128 bit value that is linked by copies to
+# %11954 (from phi elimination), %8610 (the value in the loop pre-header),
+# %8613 (defined and subreg-modified in the loop, and used after the loop)
+# and %8612:
+#
+#     %11954:vreg_128 = COPY killed %8610
+# bb.39:
+#     %8611:vreg_128 = COPY killed %11954
+#     %10280:vgpr_32 = V_LSHLREV_B32_e32 2, %8611.sub2, implicit $exec
+#     %8613:vreg_128 = COPY killed %8611
+#     %8613.sub1:vreg_128 = COPY %9719
+#     %8612:vreg_128 = COPY %8613
+#     %11954:vreg_128 = COPY killed %8612
+#     S_CBRANCH_EXECNZ %bb.39, implicit $exec
+#     S_BRANCH %bb.40
+# bb.40:
+#     undef %8615.sub0:vreg_128 = COPY killed %8613.sub0
+#
+# So this coalesces together into a single 128 bit value whose sub1 is modified
+# in the loop, but the sub2 used in the V_LSHLREV is not modified in the loop.
+#
+# The bug is that the coalesced value has a L00000004 subrange (for sub2) that
+# says that it is not live up to the end of the loop block. The symptom is that
+# Rename Independent Subregs separates sub2 into its own register, and it is
+# not live round the loop, so that pass adds an IMPLICIT_DEF for it just before
+# the loop backedge.
+
+# GCN: bb.1 (%ir-block.6):
+# GCN: V_LSHLREV_B32_e32 2, [[val:%[0-9][0-9]*]].sub2
+# GCN-NOT: [[val]]:vreg_128 = IMPLICIT_DEF
+
+--- |
+  target datalayout = "e-p:64:64-p1:64:64-p2:32:32-p3:32:32-p4:64:64-p5:32:32-p6:32:32-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024-v2048:2048-n32:64-S32-A5"
+  target triple = "amdgcn--amdpal"
+  
+  ; Function Attrs: nounwind
+  define dllexport amdgpu_cs void @_amdgpu_cs_main(i32 inreg, i32 inreg, i32 inreg, <3 x i32> inreg, i32 inreg, <3 x i32>) local_unnamed_addr {
+  .entry:
+    br label %6
+  
+  ; <label>:4206:                                   ; preds = %3820, %4206
+    %7 = bitcast i32 addrspace(3)* undef to <2 x i32> addrspace(3)*
+    %8 = bitcast i32 addrspace(3)* undef to <2 x i32> addrspace(3)*
+    %9 = bitcast i32 addrspace(3)* undef to <2 x i32> addrspace(3)*
+    %10 = bitcast i32 addrspace(3)* undef to <2 x i32> addrspace(3)*
+    %11 = bitcast i32 addrspace(3)* undef to <2 x i32> addrspace(3)*
+    %12 = bitcast i32 addrspace(3)* undef to <2 x i32> addrspace(3)*
+
+    br i1 undef, label %13, label %6
+  
+  ; <label>:4472:                                   ; preds = %4206
+    ret void
+  }
+  
+
+...
+---
+name:            _amdgpu_cs_main
+alignment:       0
+exposesReturnsTwice: false
+legalized:       false
+regBankSelected: false
+selected:        false
+failedISel:      false
+tracksRegLiveness: true
+registers:       
+  - { id: 257, class: sreg_64, preferred-register: '' }
+  - { id: 264, class: sreg_64, preferred-register: '' }
+  - { id: 4514, class: sreg_64, preferred-register: '' }
+  - { id: 4515, class: sgpr_32, preferred-register: '' }
+  - { id: 4517, class: sgpr_32, preferred-register: '' }
+  - { id: 4519, class: sgpr_32, preferred-register: '' }
+  - { id: 4521, class: sgpr_32, preferred-register: '' }
+  - { id: 4523, class: sreg_32_xm0, preferred-register: '' }
+  - { id: 4525, class: sgpr_32, preferred-register: '' }
+  - { id: 4527, class: sreg_32_xm0, preferred-register: '' }
+  - { id: 4529, class: sreg_32_xm0, preferred-register: '' }
+  - { id: 4531, class: sreg_32_xm0, preferred-register: '' }
+  - { id: 4906, class: vreg_64, preferred-register: '' }
+  - { id: 4910, class: vreg_64, preferred-register: '' }
+  - { id: 4914, class: vreg_64, preferred-register: '' }
+  - { id: 4950, class: vreg_64, preferred-register: '' }
+  - { id: 4954, class: vreg_64, preferred-register: '' }
+  - { id: 4958, class: vreg_64, preferred-register: '' }
+  - { id: 5079, class: sreg_64, preferred-register: '$vcc' }
+  - { id: 8601, class: vreg_128, preferred-register: '' }
+  - { id: 8602, class: vreg_128, preferred-register: '' }
+  - { id: 8603, class: vreg_128, preferred-register: '' }
+  - { id: 8604, class: vreg_128, preferred-register: '' }
+  - { id: 8605, class: vreg_128, preferred-register: '' }
+  - { id: 8606, class: vreg_128, preferred-register: '' }
+  - { id: 8607, class: vreg_128, preferred-register: '' }
+  - { id: 8608, class: vreg_128, preferred-register: '' }
+  - { id: 8609, class: vreg_128, preferred-register: '' }
+  - { id: 8610, class: vreg_128, preferred-register: '' }
+  - { id: 8611, class: vreg_128, preferred-register: '' }
+  - { id: 8612, class: vreg_128, preferred-register: '' }
+  - { id: 8613, class: vreg_128, preferred-register: '' }
+  - { id: 8615, class: vreg_128, preferred-register: '' }
+  - { id: 8617, class: vgpr_32, preferred-register: '' }
+  - { id: 9719, class: sreg_32, preferred-register: '' }
+  - { id: 9720, class: vreg_128, preferred-register: '' }
+  - { id: 10271, class: vreg_128, preferred-register: '' }
+  - { id: 10272, class: vreg_128, preferred-register: '' }
+  - { id: 10275, class: vgpr_32, preferred-register: '' }
+  - { id: 10280, class: vgpr_32, preferred-register: '' }
+  - { id: 10283, class: vgpr_32, preferred-register: '' }
+  - { id: 10284, class: vgpr_32, preferred-register: '' }
+  - { id: 10626, class: vreg_128, preferred-register: '' }
+  - { id: 11951, class: sreg_64, preferred-register: '' }
+  - { id: 11953, class: vreg_128, preferred-register: '' }
+  - { id: 11954, class: vreg_128, preferred-register: '' }
+frameInfo:       
+  isFrameAddressTaken: false
+  isReturnAddressTaken: false
+  hasStackMap:     false
+  hasPatchPoint:   false
+  stackSize:       0
+  offsetAdjustment: 0
+  maxAlignment:    0
+  adjustsStack:    false
+  hasCalls:        false
+  stackProtector:  ''
+  maxCallFrameSize: 4294967295
+  hasOpaqueSPAdjustment: false
+  hasVAStart:      false
+  hasMustTailInVarArgFunc: false
+  localFrameSize:  0
+  savePoint:       ''
+  restorePoint:    ''
+fixedStack:      
+stack:           
+constants:       
+body:             |
+  bb.0..entry:
+    successors: %bb.39(0x80000000)
+    liveins:
+
+    %4515:sgpr_32 = S_MOV_B32 0
+    undef %8601.sub1:vreg_128 = COPY undef %4515
+    dead %4517:sgpr_32 = S_MOV_B32 1
+    %8602:vreg_128 = COPY killed %8601
+    %8602.sub1:vreg_128 = COPY undef %4517
+    dead %4519:sgpr_32 = S_MOV_B32 2
+    %8603:vreg_128 = COPY killed %8602
+    %8603.sub1:vreg_128 = COPY undef %4519
+    dead %4521:sgpr_32 = S_MOV_B32 3
+    %8604:vreg_128 = COPY killed %8603
+    %8604.sub1:vreg_128 = COPY undef %4521
+    dead %4523:sreg_32_xm0 = S_MOV_B32 4
+    %8605:vreg_128 = COPY killed %8604
+    %8605.sub1:vreg_128 = COPY undef %4523
+    dead %4525:sgpr_32 = S_MOV_B32 5
+    %8606:vreg_128 = COPY killed %8605
+    %8606.sub1:vreg_128 = COPY undef %4525
+    dead %4527:sreg_32_xm0 = S_MOV_B32 6
+    %8607:vreg_128 = COPY killed %8606
+    %8607.sub1:vreg_128 = COPY undef %4527
+    dead %4529:sreg_32_xm0 = S_MOV_B32 7
+    %8608:vreg_128 = COPY killed %8607
+    %8608.sub1:vreg_128 = COPY undef %4529
+    %4531:sreg_32_xm0 = S_MOV_B32 255
+    %8609:vreg_128 = COPY killed %8608
+    %8609.sub1:vreg_128 = COPY %4531
+    %8610:vreg_128 = COPY killed %8609
+    %8610.sub2:vreg_128 = COPY killed %4531
+    %4514:sreg_64 = S_MOV_B64 0
+    %9719:sreg_32 = S_MOV_B32 7
+    %10272:vreg_128 = COPY undef %10626
+    %11951:sreg_64 = COPY killed %4514
+    %11953:vreg_128 = COPY killed %10272
+    %11954:vreg_128 = COPY killed %8610
+  
+  bb.39 (%ir-block.6):
+    successors: %bb.40(0x04000000), %bb.39(0x7c000000)
+  
+    %8611:vreg_128 = COPY killed %11954
+    %10271:vreg_128 = COPY killed %11953
+    %257:sreg_64 = COPY killed %11951
+
+    %10280:vgpr_32 = V_LSHLREV_B32_e32 2, %8611.sub2, implicit $exec
+    %10284:vgpr_32 = V_ADD_I32_e32 1152, %10280, implicit-def dead $vcc, implicit $exec
+    $m0 = S_MOV_B32 -1
+    %4906:vreg_64 = DS_READ2_B32 killed %10284, 0, 1, 0, implicit $m0, implicit $exec :: (load 8 from %ir.7, align 4, addrspace 3)
+    %4910:vreg_64 = DS_READ2_B32 %10280, -112, -111, 0, implicit $m0, implicit $exec :: (load 8 from %ir.8, align 4, addrspace 3)
+    %4914:vreg_64 = DS_READ2_B32 %10280, 0, 1, 0, implicit $m0, implicit $exec :: (load 8 from %ir.9, align 4, addrspace 3)
+
+    %10283:vgpr_32 = V_ADD_I32_e32 1160, %10280, implicit-def dead $vcc, implicit $exec
+    %4950:vreg_64 = DS_READ2_B32 killed %10283, 0, 1, 0, implicit $m0, implicit $exec :: (load 8 from %ir.10, align 4, addrspace 3)
+    %4954:vreg_64 = DS_READ2_B32 %10280, -110, -109, 0, implicit $m0, implicit $exec :: (load 8 from %ir.11, align 4, addrspace 3)
+    %4958:vreg_64 = DS_READ2_B32 %10280, 2, 3, 0, implicit $m0, implicit $exec :: (load 8 from %ir.12, align 4, addrspace 3)
+
+    undef %9720.sub1:vreg_128 = COPY undef %9719
+    %8613:vreg_128 = COPY killed %8611
+    %8613.sub1:vreg_128 = COPY %9719
+    %10275:vgpr_32 = V_ADD_I32_e32 1, %10271.sub0, implicit-def dead $vcc, implicit $exec
+    %5079:sreg_64 = V_CMP_LT_I32_e64 5, %10275, implicit $exec
+    %264:sreg_64 = S_OR_B64 killed %5079, killed %257, implicit-def $scc
+    %8612:vreg_128 = COPY %8613
+    %11951:sreg_64 = COPY %264
+    %11953:vreg_128 = COPY %9720
+    %11954:vreg_128 = COPY killed %8612
+    $exec = S_ANDN2_B64_term $exec, %264
+    S_CBRANCH_EXECNZ %bb.39, implicit $exec
+    S_BRANCH %bb.40
+  
+  bb.40 (%ir-block.13):
+  
+    $exec = S_OR_B64 $exec, killed %264, implicit-def $scc
+    %8617:vgpr_32 = V_MOV_B32_e32 0, implicit $exec
+    undef %8615.sub0:vreg_128 = COPY killed %8613.sub0
+    %8615.sub2:vreg_128 = COPY %8617
+    S_ENDPGM
+
+...