PATCH: RegisterCoalescer: Fix bug when rematerializing instsructions with subregs
tom at stellard.net
Tue Apr 22 07:24:04 PDT 2014
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
There is an assertion at RegisterCoalescer.cpp:823, which prevents rematerialzed
instructions from having implicit uses. Why is this necessary? Can this be removed?
I have attached a patch which gets rid of it, but I'm not sure it is correct.
> > 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 --------------
A non-text attachment was scrubbed...
Size: 1859 bytes
Desc: not available
More information about the llvm-commits