PATCH: RegisterCoalescer: Fix bug when rematerializing instsructions with subregs

[Quentin Colombet]

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.


>  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?

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>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140422/89dd2f36/attachment.html>