This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Refactor ds_read/ds_write related select code for better readability.
ClosedPublic

Authored by hsmhsm on Apr 12 2021, 4:29 AM.

Details

Summary

Part of the code related to ds_read/ds_write ISel is refactored, and the
corresponding comment is re-written for better readability, which would help
while implementing any future ds_read/ds_write ISel related modifications.

Diff Detail

Event Timeline

hsmhsm created this revision.Apr 12 2021, 4:29 AM
hsmhsm requested review of this revision.Apr 12 2021, 4:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 12 2021, 4:29 AM
arsenm added inline comments.Apr 12 2021, 5:16 AM
llvm/lib/Target/AMDGPU/SIISelLowering.cpp
1415–1416

"related hardware bug" - no idea what this is referring to

hsmhsm added inline comments.Apr 12 2021, 7:39 AM
llvm/lib/Target/AMDGPU/SIISelLowering.cpp
1415–1416

This is actually referring to - https://github.com/llvm/llvm-project/blob/6c0a1ed3a94ff34e6d9500cdfd04858b1a6f72aa/llvm/lib/Target/AMDGPU/AMDGPU.td#L175

The boolean function hasLDSMisalignedBug() is used to track it.

rampitec added inline comments.Apr 12 2021, 11:27 AM
llvm/lib/Target/AMDGPU/SIISelLowering.cpp
1431

It drops the comment about gfx8 and older which is probably still relevant.

1439

Same here.

hsmhsm updated this revision to Diff 337097.Apr 13 2021, 4:00 AM

Fixed review comments by Stas.

hsmhsm marked 3 inline comments as done.Apr 13 2021, 4:01 AM
rampitec accepted this revision.Apr 15 2021, 1:21 PM
This revision is now accepted and ready to land.Apr 15 2021, 1:21 PM
foad added inline comments.Apr 19 2021, 2:34 AM
llvm/lib/Target/AMDGPU/DSInstructions.td
843–847

Please just merge these two foreach loops. Same for the other pairs of foreach loops which are now adjacent.

880

I think something like "do not select ds_read_b128/ds_write_b128 for unaligned accesses" would be more accurate. We still select them for aligned accesses.

Also "at lower alignments" doesn't make much sense now you have changed the first half of the comment not to mention any particular alignment.