[PATCH] D82788: AMDGPU: Fix alignment requirements for 96bit and 128bit local loads and stores

[Mirko Brkusanin via Phabricator via llvm-commits]

mbrkusanin marked an inline comment as done.
mbrkusanin added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:1377
+      // ds_read/write_b96 require 16-byte alignment. gfx9 and onward has
+      // unaligned access support but windows likes to keep it dword aligned.
+      bool IsGFX9Plus = Subtarget->getGeneration() >= AMDGPUSubtarget::GFX9;
----------------
nhaehnle wrote:
> mbrkusanin wrote:
> > nhaehnle wrote:
> > > arsenm wrote:
> > > > nhaehnle wrote:
> > > > > arsenm wrote:
> > > > > > This is a windows driver bug and doesn't deserve mentioning here. We do not know what the host OS is
> > > > > I agree that it's a bug, but I find it reasonable to mention it here. I'd change the comment though to specifically call out that this should be considered a bug in the Windows KMD.
> > > > It's not specific to this instance though, this belongs with the place where we assume unaligned access for amdhsa
> > > Okay, that's fair.
> > Is the new wording ok?
> No, it makes an incorrect statement. The alignment requirement only exists up to gfx8, so it should say "... require 16-byte alignment on gfx8 and older".
> 
> I also don't think we enforce dword alignment on gfx9+ anywhere for any other kind of memory access, so we shouldn't do it here. The mess on Windows is a problem, but if we really need to work around it, it should be a target "feature" (misfeature, really, but oh well).
So you're recommending two new features? One to allow misaligned DS instructions and one that requires dword alignment (currently hasUnalignedDSAccess but should be renamed in this case) alongside default requirement of 16 byte?


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

https://reviews.llvm.org/D82788