This is an archive of the discontinued LLVM Phabricator instance.

[RegisterCoalescer] Resolve conflict based on liveness of subregister
ClosedPublic

Authored by ruiling on Jun 18 2021, 12:00 AM.

Details

Summary

Currently we are resolving lane/subregister conflict by visiting
instructions sequentially in current block to see whether there is any
use of the tainted lanes. To save compile time, we are not doing further
check in successor blocks. This sounds reasonable without subgregister liveness.

But since we have added subregister liveness tracking capability to
register coalescer, we can easily determine whether we have subregister
liveness conflict by checking subranges. This would help coalescing more
COPYs for target that enable subregister tracking.

Diff Detail

Event Timeline

ruiling created this revision.Jun 18 2021, 12:00 AM
ruiling requested review of this revision.Jun 18 2021, 12:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 18 2021, 12:00 AM
foad added a subscriber: foad.Jun 18 2021, 6:06 AM

I don’t know much about the RegisterCoalescer, but I like the assembly changes.
(There’s a typo in the title: conflict)

ruiling retitled this revision from [RegisterCoalescer] Resolve confict based on liveness of subregister to [RegisterCoalescer] Resolve conflict based on liveness of subregister.Jun 22 2021, 8:07 PM

Hi @arsenm, @qcolombet @kparzysz, do you have any concern over the approach?

arsenm added inline comments.Jun 28 2021, 6:21 AM
llvm/lib/CodeGen/RegisterCoalescer.cpp
2896

What's the point of entering the loop if this starts out as true? Can't you just early return here?

2903–2904

return ClobberOther ? CR_Impossible : CR_Replace

llvm/test/CodeGen/AMDGPU/splitkit-getsubrangeformask.ll
354–355

I'm a bit worried this test is no longer hitting what it was supposed to be testing

ruiling updated this revision to Diff 355094.Jun 28 2021, 7:45 PM

Update logic for !OtherLI.hasSubRanges().
Also address review comments as well.

ruiling added inline comments.Jun 28 2021, 7:53 PM
llvm/lib/CodeGen/RegisterCoalescer.cpp
2896

I have refined the logic for !OtherLI.hasSubranges() because I think they are still coalescable if there LaneMask don't have any common bit.

2903–2904

I have just moved "return CR_Impossible;" inside the loop.

llvm/test/CodeGen/AMDGPU/splitkit-getsubrangeformask.ll
354–355

I have not check into details, I am not sure @foad Do you have any idea on this?

qcolombet accepted this revision.Jun 29 2021, 11:03 AM

Looks fine to me but please wait on the comment from Jay (@foad ) and Matt (@arsenm ) before pushing.
Nitpick below.

llvm/lib/CodeGen/RegisterCoalescer.cpp
2921

No else after return. This would also avoid the increased indentation here.

This revision is now accepted and ready to land.Jun 29 2021, 11:03 AM

LGTM with typo / indentation fixed

llvm/lib/CodeGen/RegisterCoalescer.cpp
2896

Grammar, s/has/have/

ruiling added inline comments.Jul 6 2021, 1:04 AM
llvm/test/CodeGen/AMDGPU/splitkit-getsubrangeformask.ll
354–355

I tried below steps hoping to hitting the issue this was used to test (I have revert @foad's change to make it assert). But I failed to reproduce with my change.

llc -march=amdgcn -mcpu=gfx1010 -stop-before=greedy llvm/test/CodeGen/AMDGPU/splitkit-getsubrangeformask.ll -o 1.mir
llc -march=amdgcn -mcpu=gfx1010 -run-pass=greey 1.mir

I have also tried various ways to manually edit the .ll file, also failed to trigger the old issue. Any suggestion?

foad added inline comments.Jul 6 2021, 3:52 AM
llvm/test/CodeGen/AMDGPU/splitkit-getsubrangeformask.ll
354–355

Sorry, I don't know how to trigger the old issue.

ruiling updated this revision to Diff 357186.Jul 8 2021, 3:57 AM

Address review comments and add some TODO to the affected test splitkit-getsubrangeformask.ll.
I failed to find other way to reproduce the issue that splitkit-getsubrangeformask.ll used to hit.
I hope I can find ways to reproduce the issue when I get more knowledge of LiveRange split.

I am going to push this version if no objection.

This revision was landed with ongoing or failed builds.Jul 13 2021, 11:44 PM
This revision was automatically updated to reflect the committed changes.