PATCH: RegisterCoalescer: Fix bug when rematerializing instsructions with subregs

[Tom Stellard]

On Tue, Apr 22, 2014 at 10:04:24AM -0700, Quentin Colombet wrote:
> 
> On Apr 22, 2014, at 7:24 AM, Tom Stellard <tom at stellard.net> wrote:
> 
> > On Mon, Apr 21, 2014 at 09:38:02AM -0700, Quentin Colombet wrote:
> >> 
> >> On Apr 21, 2014, at 6:49 AM, Tom Stellard <tom at stellard.net> wrote:
> >> 
> >>> On Fri, Apr 18, 2014 at 03:43:55PM -0700, Quentin Colombet wrote:
> >>>> Hi Tom,
> >>>> 
> >>>> I’ve looked at your patch and it looks like a workaround to a more general issue.
> >>>> 
> >>>> I’ve applied your second patch and looked at what happen with the rematerialization in that case.
> >>>> 
> >>>> Unless I am not looking at the right fragment, the copy you are trying to avoid to rematerialize is:
> >>>> %vreg169<def> = COPY %vreg91:sub0_sub1; SReg_64:%vreg169 SReg_128:%vreg91
> >>>> 
> >>>> And the related rematerializable definition is:
> >>>> %vreg91:sub0_sub1<def,read-undef> = S_MOV_B64 0; SReg_128:%vreg91
> >>>> 
> >>>> Thus, one would expect to rematerialize this into:
> >>>> %vreg169:sub0_sub1<def,read-undef> = S_MOV_B64 0; SReg_128:%vreg169
> >>>> 
> >>>> Which the current code should correctly handle.
> >>>> 
> >>>> However, in your case, the instruction that have been picked up for rematerialization is:
> >>>> %vreg91:sub3<def> = S_MOV_B32 61440; SReg_128:%vreg91
> >>>> 
> >>>> Which is the same vreg91, but not the right subregister. So looks like there is a liveness or liveness-update bug with the subregisters.
> >>>> 
> >>> 
> >>> Are you sure LLVM ToT does sub-register liveness tracking?
> >> No, I am not sure, but the root cause of the bug is this liveness tracking.
> >> So I’d suggest, go for your workaround (with the fixed typo) and file a PR.
> >> 
> > 
> > I can commit this fix, but I've found another issue that needs to be resolved
> > first.
> > 
> > There is an assertion at RegisterCoalescer.cpp:823, which prevents rematerialzed
> > instructions from having implicit uses.  Why is this necessary?
> The rematerialization mechanism assumes that each argument is a constant of some sort.
> If that is not the case, e.g., with an implicit use, you cannot guarantee that rematerializing is correct.
> 
> E.g.,
> 
> v1 = remat <imp-use R1>
> R1 = something
> v2 = copy v1
> =>
> R1 = something
> v2 = remat <imp-use R1> ; # the value in R1 is not the same than the value used to rematerialize v1. Therefore this may not be correct.
> 

Ok, I understand.

> 
> >  Can this be removed?
> No, it shouldn’t be removed.
> If the implicit use does not change the rematerialized value, maybe you shouldn’t have an implicit use on that instruction in the first place.
> 
> What about the original issue, the liveness problem, did you sort it out?
> 

I think the problem may simply be that the coalescer assumes sub-register
liveness tracking when it doesn't exist yet, but I'm not completely sure.

-Tom

> Thanks,
> -Quentin
> 
> > I have attached a patch which gets rid of it, but I'm not sure it is correct.
> > 
> > -Tom
> > 
> >> Thanks,
> >> -Quentin
> >> 
> >>> I thought liveness was
> >>> tracked by super-register only.
> >>> 
> >>> -Tom
> >>> 
> >>>> Could you please track this down to make sure we understand the problem?
> >>>> 
> >>>> Now, in the mean time, we may want to provide a workaround (and fix a PR to remember we added a workaround).
> >>>> Your workaround is appropriate with the following fix:
> >>>> +  // The COPY src reg and the DefMI dst reg must have the same subreg index
> >>>> +  if (CopyMI->getOperand(1).getSubReg() != DefMI->getOperand(0).getSubReg())
> >>>> +    return false;
> >>>> 
> >>>> I.e., instead of checking the sub register of CopyDstOperand, you have to check the sub register of copy *src* operand (like you said in your comment), i.e., 1 instead of 0.
> >>>> 
> >>>> Thanks,
> >>>> -Quentin
> >>>> 
> >>>> On Apr 16, 2014, at 3:13 PM, Tom Stellard <tom at stellard.net> wrote:
> >>>> 
> >>>>> Hi,
> >>>>> 
> >>>>> This patch fixes a bug in the register coalescer where it was incorrectly
> >>>>> rematerializing instructions that define sub-regs.
> >>>>> 
> >>>>> I've also attached the R600 patch which uncovers this bug.
> >>>>> 
> >>>>> Please Review.
> >>>>> 
> >>>>> -Tom
> >>>>> <0001-RegisterCoalescer-Fix-bug-when-rematerializing-insts.patch><0002-R600-SI-Improve-chances-of-rematerializing-during-re.patch>_______________________________________________
> >>>>> llvm-commits mailing list
> >>>>> llvm-commits at cs.uiuc.edu
> >>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> >> 
> > <0001-XXX-Fix-assertion-in-coalescer.patch>
>