This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Set DS alignment requirements to be more strict
ClosedPublic

Authored by mbrkusanin on Sep 17 2020, 2:53 AM.

Details

Summary

Alignment requirements for ds_read/write_b96/b128 for gfx9 and onward are
now the same as for other GCN subtargets. This way we can avoid any
unintentional use of these instructions on systems that do not support dword
alignment and instead require natural alignment.
This also makes 'SH_MEM_CONFIG.alignment_mode == STRICT' the default.

Diff Detail

Event Timeline

mbrkusanin created this revision.Sep 17 2020, 2:53 AM
mbrkusanin requested review of this revision.Sep 17 2020, 2:53 AM

This helps vulkan gfx9 windows tests.

From the description, I don't understand what this is trying to fix

llvm/lib/Target/AMDGPU/SIISelLowering.cpp
1438–1439

I think the logic for 2 byte alignment not being fast also applies here, but that's a separate change

1467–1468

This looks wrong to me, 4 byte alignment is still usable?

llvm/test/CodeGen/AMDGPU/ds_write2.ll
2

What is -dword-access-mode?

mbrkusanin added inline comments.Sep 17 2020, 6:47 AM
llvm/lib/Target/AMDGPU/SIISelLowering.cpp
1467–1468

It was when we allowed dword alignment. But now we want strict to be default (because of windows). So now it's always 8 (because of read2/write2), unless it's +unaligned-access-mode.

llvm/test/CodeGen/AMDGPU/ds_write2.ll
2

Sorry, that should have been removed.
With this patch we'll have:
alignment_mode = strict (default)
alignment_mode = unaligned (with +unaligned-access-mode)

That option was supposed to represent "alignment_mode = dword" (which was the default before) but we decided against that, at least in this patch.

  • Removed dword-access-mode from tests.
arsenm added inline comments.Sep 17 2020, 6:53 AM
llvm/test/CodeGen/AMDGPU/GlobalISel/legalize-load-local.mir
8

This test should probably have checks with unaligned access enabled

  • Added run line with -mattr=+unaligned-access-mode to test/CodeGen/AMDGPU/GlobalISel/legalize-load-local.mir
arsenm added inline comments.Sep 17 2020, 8:28 AM
llvm/lib/Target/AMDGPU/SIISelLowering.cpp
1439

Can you add a fixme to not report fast for 2 byte alignment?

llvm/test/CodeGen/AMDGPU/GlobalISel/lds-misaligned-bug.ll
4

This should also probably have some checks with unaligned access enabled

mbrkusanin marked 2 inline comments as done.
arsenm accepted this revision.Sep 18 2020, 6:15 AM
This revision is now accepted and ready to land.Sep 18 2020, 6:15 AM