Page MenuHomePhabricator

Do not abort on unresolved conflicts when joining subranges
AbandonedPublic

Authored by kparzysz on Aug 26 2016, 12:56 PM.

Details

Summary

These conflicts can legitimately happen when merging registers where subregister definitions of one taint, but not invalidate the other. While the main range conflict resolution can deal with it, the subrange conflicts will remain due to the overlapping segments.

Diff Detail

Repository
rL LLVM

Event Timeline

kparzysz updated this revision to Diff 69424.Aug 26 2016, 12:56 PM
kparzysz retitled this revision from to Do not abort on unresolved conflicts when joining subranges.
kparzysz updated this object.
kparzysz added a reviewer: MatzeB.
kparzysz set the repository for this revision to rL LLVM.
kparzysz added a subscriber: llvm-commits.
kparzysz edited edge metadata.Aug 26 2016, 12:59 PM
kparzysz added a subscriber: michel.daenzer.
MatzeB edited edge metadata.Aug 26 2016, 10:56 PM

This should ideally have a .mir test with a minimum reproducer. Maybe it is possible to make an actual test out of the comment in the code (see for example the other .mir tests in the AMDGPU directory on how you can model nearly all situations by appending implicit defs/uses to NOP instructions).

lib/CodeGen/RegisterCoalescer.cpp
2342–2377

I am trying to understand this, let's look at :sub1 for example: It is defined in 736 and read in 976, in that time :sub1 of vreg140 cannot be live or the join would be invalid. After that :sub1 is written in 1008 and read in 1248, in this time vreg131:sub1 cannot be live or the join would be invalid. I don't see an overlap in the subregisters here.

kparzysz added inline comments.Aug 29 2016, 5:37 AM
lib/CodeGen/RegisterCoalescer.cpp
2342–2377

Sub1 may not be the best choice here. Consider sub2 instead: vreg140:sub2 is first defined in 704, then it is live until 1008 and subsequently overwritten in 1040. The live subrange for it will be [704,1008)[1040,1248). This will overlap with the live range for vreg131:sub2. The join of vreg131 and vreg140 is valid, since all the lanes written via vreg131:subN = COPY ... are never actually used via vreg140:subN. They are considered "used" for the purpose of liveness tracking, but are not actually used by the program. The function resolveConflicts checks specifically for that (via taintExtent and usesLanes).

kparzysz updated this revision to Diff 69572.Aug 29 2016, 8:34 AM
kparzysz edited edge metadata.

Created a reduced .mir testcase.

qcolombet added inline comments.Sep 1 2016, 4:40 PM
lib/CodeGen/RegisterCoalescer.cpp
2342–2377

I would not expect the example in the comment to pass the usesLanes checks because of the lack of undef flags, right?
I.e., that does not seem correct to jump to the Replace conclusion.

They are considered "used" for the purpose of liveness tracking, but are not actually used by the program.

But if they are alive that means they are used right? I mean we couldn't merge them unless vreg140:sub1 has read-undef, could we?

Basically, I don't like us making assumptions on what the liveness models compared to what the program actually uses. I am afraid it may work for some targets, but not all of them. Therefore, I would rather be conservatively correct (that we have right now) than guessing around the liveness.

kparzysz abandoned this revision.Sep 2 2016, 7:02 AM

The problem is somewhere else: live subranges should not be extended to other subregs' defs.

lib/CodeGen/RegisterCoalescer.cpp
2342–2377

My previous comments are incorrect. The problem is in the fact that live subranges were extended to other subregs' defs. I will submit another patch that corrects that issue.