PATCH: RegisterCoalescer: Fix bug when rematerializing instsructions with subregs

[Quentin Colombet]

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.

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140421/33c26830/attachment.html>