This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Fix bitcast v4i64/v16i16
ClosedPublic

Authored by piotr on Jul 8 2022, 8:18 AM.

Details

Summary

Fix a regression introduced in D128865.

Diff Detail

Event Timeline

piotr created this revision.Jul 8 2022, 8:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 8 2022, 8:18 AM
piotr requested review of this revision.Jul 8 2022, 8:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 8 2022, 8:18 AM
piotr added a comment.Jul 8 2022, 8:25 AM

While this fixes a regression in an important benchmark, I think we need to re-think how these BitConverts are specified in a follow-up work, as the current list has been highly inconsistent, even before my changes.

I guess, the list has grown organically from the cases that isel needs supporting. Does anyone know if we should be listing all possible cases of a bitcast for a given width? And always have both SReg/VReg? Or it's fine to have a list like specified currently?

arsenm added inline comments.Jul 8 2022, 8:27 AM
llvm/test/CodeGen/AMDGPU/bitcast-v4i64-v16i16.ll
30–31

Don't you need just this part and the other stores? Why the control flow?

piotr added inline comments.Jul 8 2022, 9:01 AM
llvm/test/CodeGen/AMDGPU/bitcast-v4i64-v16i16.ll
30–31

Looks the failing pattern is very sensitive to the IR coming to the isel. I couldn't repro this with only one bitcast either.

arsenm added inline comments.Jul 8 2022, 9:04 AM
llvm/test/CodeGen/AMDGPU/bitcast-v4i64-v16i16.ll
30–31

Merge with amdgcn.bitcast.ll?

I think the trick is to have the value defined in a different block and to not have it sink. You can reduce the block count

piotr updated this revision to Diff 443396.Jul 8 2022, 5:20 PM

Good tip, thanks! Added a couple of tests in bitcast.ll, and reverted an erroneous edit from the original commit that the tests uncovered.

arsenm accepted this revision.Jul 11 2022, 11:36 AM
This revision is now accepted and ready to land.Jul 11 2022, 11:36 AM
This revision was landed with ongoing or failed builds.Jul 11 2022, 1:32 PM
This revision was automatically updated to reflect the committed changes.