This is an archive of the discontinued LLVM Phabricator instance.

[amdgpu] Enable selection of `s_cselect_b64`.
ClosedPublic

Authored by hliao on Sep 2 2021, 7:56 AM.

Diff Detail

Event Timeline

hliao created this revision.Sep 2 2021, 7:56 AM
hliao requested review of this revision.Sep 2 2021, 7:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 2 2021, 7:56 AM
rampitec added inline comments.Sep 2 2021, 9:34 AM
llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
6324

Bail before you have created new virtual registers.

arsenm added a comment.Sep 2 2021, 9:59 AM

Can you add a gfx90a test where it ends up needing to be moved to the VALU to make sure that works? I think you can get away with using the unaligned class here as is

hliao updated this revision to Diff 370342.Sep 2 2021, 11:42 AM

Revise select64.ll test

hliao marked an inline comment as done.Sep 2 2021, 11:43 AM

Can you add a gfx90a test where it ends up needing to be moved to the VALU to make sure that works? I think you can get away with using the unaligned class here as is

auto-gen checks for select64.ll with extra gfx90a target.

This revision is now accepted and ready to land.Sep 2 2021, 11:44 AM
hliao updated this revision to Diff 370352.Sep 2 2021, 11:59 AM

Update another test where s_cselect is moved to VALU

This revision was landed with ongoing or failed builds.Sep 7 2021, 7:45 AM
This revision was automatically updated to reflect the committed changes.
foad added a comment.Sep 7 2021, 7:54 AM

Just for the record, see also this comment from D82370:

Additionally, remove pattern for selects with 64-bit
inputs, which are rare, because handling them properly
requires more thought.

This reverted part of D81925, which originally added both 32- and 64-bit patterns.

hliao added a comment.Sep 7 2021, 8:06 AM

Just for the record, see also this comment from D82370:

Additionally, remove pattern for selects with 64-bit
inputs, which are rare, because handling them properly
requires more thought.

This reverted part of D81925, which originally added both 32- and 64-bit patterns.

In fact, we found that 64-bit select is not rare, partially due to the use of 64-bit pointers (tend to be uniform in most cases.) This patch is developed to address that pending issue in D81925.

piotr added a comment.Sep 7 2021, 9:03 AM

Just for the record, see also this comment from D82370:

Additionally, remove pattern for selects with 64-bit
inputs, which are rare, because handling them properly
requires more thought.

This reverted part of D81925, which originally added both 32- and 64-bit patterns.

In fact, we found that 64-bit select is not rare, partially due to the use of 64-bit pointers (tend to be uniform in most cases.) This patch is developed to address that pending issue in D81925.

Thanks for working on this!
The observation I made about 64-bit inputs being rare was based on graphics content. The changes I did in D81925 in si-fix-sgpr-copies did not universally work for 64-bit selects, so I removed that part in D82370.

llvm/test/CodeGen/AMDGPU/addrspacecast.ll