This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Support unaligned flat scratch in TLI
ClosedPublic

Authored by rampitec on Dec 21 2020, 4:01 PM.

Details

Summary

Adjust SITargetLowering::allowsMisalignedMemoryAccessesImpl for
unaligned flat scratch support. Mostly needed for global isel.

Diff Detail

Event Timeline

rampitec created this revision.Dec 21 2020, 4:01 PM
rampitec requested review of this revision.Dec 21 2020, 4:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 21 2020, 4:01 PM
Herald added a subscriber: wdng. · View Herald Transcript
arsenm added inline comments.Dec 22 2020, 7:00 AM
llvm/lib/Target/AMDGPU/SIISelLowering.cpp
1479

Does it really only work if accessed through the flat instructions? I thought it would even through MUBUF

rampitec marked an inline comment as done.Dec 22 2020, 10:34 AM
rampitec added inline comments.
llvm/lib/Target/AMDGPU/SIISelLowering.cpp
1479

Yes, it is only flat scratch. For MUBUF spec says: "Swizzled addressing also requires dword aligned accesses".

I think there's some missing tests, these test changes look incidental. Can you add some checks to the existing unaligned load/store base tests

rampitec marked an inline comment as done.Dec 22 2020, 1:20 PM

I think there's some missing tests, these test changes look incidental. Can you add some checks to the existing unaligned load/store base tests

Which tests do you mean? As far as I understand this mostly affects GlobalISel and child patch D93670 contains the actual test, all unaligned cases.

I think there's some missing tests, these test changes look incidental. Can you add some checks to the existing unaligned load/store base tests

Which tests do you mean? As far as I understand this mostly affects GlobalISel and child patch D93670 contains the actual test, all unaligned cases.

This should cover both. I mean unaligned-load-store.ll (and some others, it's annoying how the test names for these aren't consistent and there is redundancy between files)

I think there's some missing tests, these test changes look incidental. Can you add some checks to the existing unaligned load/store base tests

Which tests do you mean? As far as I understand this mostly affects GlobalISel and child patch D93670 contains the actual test, all unaligned cases.

This should cover both. I mean unaligned-load-store.ll (and some others, it's annoying how the test names for these aren't consistent and there is redundancy between files)

It should not change without flat scratch enabled?

I think there's some missing tests, these test changes look incidental. Can you add some checks to the existing unaligned load/store base tests

Which tests do you mean? As far as I understand this mostly affects GlobalISel and child patch D93670 contains the actual test, all unaligned cases.

This should cover both. I mean unaligned-load-store.ll (and some others, it's annoying how the test names for these aren't consistent and there is redundancy between files)

It should not change without flat scratch enabled?

Or do you want me to add flat scratch to the unaligned-load-store.ll?

I think there's some missing tests, these test changes look incidental. Can you add some checks to the existing unaligned load/store base tests

Which tests do you mean? As far as I understand this mostly affects GlobalISel and child patch D93670 contains the actual test, all unaligned cases.

This should cover both. I mean unaligned-load-store.ll (and some others, it's annoying how the test names for these aren't consistent and there is redundancy between files)

It should not change without flat scratch enabled?

Or do you want me to add flat scratch to the unaligned-load-store.ll?

Yes, I think we're missing some testing combinations (not helped by the mess from the old tests)

rampitec updated this revision to Diff 313428.Dec 22 2020, 1:54 PM

Added run line to the unaligned-load-store.ll.

arsenm accepted this revision.Dec 22 2020, 3:36 PM
This revision is now accepted and ready to land.Dec 22 2020, 3:36 PM
This revision was automatically updated to reflect the committed changes.