[PATCH] D99352: [AMDGPU] ds_read_*/ds_write_* operations require strict alignment.

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:1435
+      // ds_read/write_b128 require 16-byte alignment on gfx8 and older.
+      bool Aligned = Alignment >= Align(16);
       if (IsFast)
----------------
rampitec wrote:
> arsenm wrote:
> > hsmhsm wrote:
> > > arsenm wrote:
> > > > I don't like changing this function to lie and say it doesn't work when it does
> > > My understanding is that the function allowsMisalignedMemoryAccessesImpl() should check for the strict alignment requirment for ds_read/ds_write operations. I am now further updated this function accordingly.
> > You are making this stricter than the instructions really allow
> That is more or less the point. I think there is some balance here between the perf drop when a wide access is used for misaligned data and perf loss when a narrow access is used on data for which we cannot prove alignment but it just happens to be aligned.
> 
> This seems to warrant a perf testing on this patch. @cfang does this solve perf regression you were working on?
The function reports if it's legal and works. IsFast is separate and can change, but it should still report this as a legal unaligned access


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99352