Page MenuHomePhabricator

PostRA Machine Sink should take care of COPY defining register that is a sub-register by another COPY source operand
ClosedPublic

Authored by alex-t on Dec 6 2019, 10:23 AM.

Details

Summary

This pass was created with the assumption that all the copy sequences like

x = COPY y
z = COPY x

were already handled by the coalescer and transformed to

z = COPY y

This was true until the sub-registers appeared.

Considering the following pattern:

x = COPY y
z = COPY s
w = COPY x_z

where x_z is a super-reg consisting of x and y

Current implementation does not take into account registers defined by another copies assuming that one COPY cannot define register that is used by another one. As a result in sinks the COPY defining sub-register to one successor but COPY that reads super-register to another providing this later with uninitialized register.
Proposed fix updates lane mask after sinking the COPY rather then merely add whole COPY source to live ins.

Diff Detail

Event Timeline

alex-t created this revision.Dec 6 2019, 10:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 6 2019, 10:24 AM

Is there a test missing?

With this change, it looks as though accumulateUsedDefed is called multiple times on some paths. That seems wrong. It does seem plausible that we'd have to keep accumulating for all code paths.

Also, this needs a test.

alex-t added a comment.EditedDec 9 2019, 6:04 AM

With this change, it looks as though accumulateUsedDefed is called multiple times on some paths. That seems wrong.

Yes. In fact this fix is wrong. Will be looking for another way to handle this.

arsenm requested changes to this revision.Dec 10 2019, 8:18 AM
This revision now requires changes to proceed.Dec 10 2019, 8:18 AM
alex-t updated this revision to Diff 233443.Dec 11 2019, 1:37 PM
alex-t retitled this revision from PostRA Machine Sink should count COPY defining register used by another COPY to PostRA Machine Sink should take care of COPY defining register that is a sub-register by another COPY source operand.
alex-t edited the summary of this revision. (Show Details)

Just correct solution + tests

Almost LGTM. Do you need those liveins reorderings?

llvm/test/CodeGen/AArch64/bisect-post-ra-machine-sink.mir
12

is this really needed?

alex-t marked an inline comment as done.Dec 12 2019, 8:38 AM
alex-t added inline comments.
llvm/test/CodeGen/AArch64/bisect-post-ra-machine-sink.mir
12

sortUniqueLiveIns does sort. Hence we're going to have w0 before w1

alex-t marked an inline comment as done.Dec 12 2019, 8:38 AM
vpykhtin accepted this revision.Dec 12 2019, 9:01 AM

LGTM.

arsenm added inline comments.Dec 13 2019, 10:06 AM
llvm/lib/CodeGen/MachineSink.cpp
1220

Should use MCRegUnitMaskIterator instead of getting the lane masks for each subreg

alex-t updated this revision to Diff 233847.Dec 13 2019, 12:21 PM

Updated according to reviewer request

alex-t marked an inline comment as done.Dec 13 2019, 12:21 PM
This revision was not accepted when it landed; it landed in state Needs Review.Dec 17 2019, 4:25 AM
This revision was automatically updated to reflect the committed changes.