[PATCH] D98491: [AMDGPU] Split GCN subtarget features for unaligned access

[Mahesha S via Phabricator via llvm-commits]

hsmhsm added a comment.

In D98491#2645085 <https://reviews.llvm.org/D98491#2645085>, @hsmhsm wrote:

> In D98491#2644858 <https://reviews.llvm.org/D98491#2644858>, @rampitec wrote:
>
>> In D98491#2643944 <https://reviews.llvm.org/D98491#2643944>, @hsmhsm wrote:
>>
>>> @mbrkusanin
>>> My proposal was simple: prefer ds_read2_b64 (and write) over b128 if alignment < 16. I never heard of b64 performance issues though, so the rest is the same as now: create a widest load/store possible with that one exception for b128.
>>
>> We have since then confirmed that ds_read_b64 has the same performance hit on memory not aligned to 64 bit, so 64 bit operations too need an alignment check.
>
> But I see the below ISel pattern for - `DS_READ_B64` indeed checks alignment requirment, though not sure if it is  over written some where else.
>
> DSInstructions.td:717-719
>
>   foreach vt = VReg_64.RegTypes in {
>   defm : DSReadPat_mc <DS_READ_B64, vt, "load_align8_local">;
>   }

And further based on a closer look at the code, I think, in general, we better revert the commit 0654ff703d4e99423133165db63083b831efb9b6 <https://reviews.llvm.org/rG0654ff703d4e99423133165db63083b831efb9b6> in order to avoid above performance issues because of unligned access.  But, I am not aware of the consequences of reverting this patch.

@mbrkusanin / @foad What is your openion on reverting the patch 0654ff703d4e99423133165db63083b831efb9b6 <https://reviews.llvm.org/rG0654ff703d4e99423133165db63083b831efb9b6>?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98491/new/

https://reviews.llvm.org/D98491