[PATCH] D87821: [AMDGPU] Set DS alignment requirements to be more strict

Thu Sep 17 06:47:32 PDT 2020

mbrkusanin added inline comments.

================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:1467-1468
       // ds_read2/write2_b64.
-      bool Aligned = Alignment >= Align((Subtarget->hasUnalignedDSAccess() &&
-                                         !Subtarget->hasLDSMisalignedBug())
-                                            ? 4
----------------
arsenm wrote:
> This looks wrong to me, 4 byte alignment is still usable?
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.

================
Comment at: llvm/test/CodeGen/AMDGPU/ds_write2.ll:2
 ; RUN: llc -march=amdgcn -mcpu=bonaire -verify-machineinstrs -mattr=+load-store-opt < %s | FileCheck -enable-var-scope -strict-whitespace -check-prefixes=GCN,CI %s
-; RUN: llc -march=amdgcn -mcpu=gfx900 -verify-machineinstrs -mattr=+load-store-opt,+flat-for-global,-unaligned-access-mode < %s | FileCheck -enable-var-scope -strict-whitespace -check-prefixes=GCN,GFX9,GFX9-ALIGNED %s
+; RUN: llc -march=amdgcn -mcpu=gfx900 -verify-machineinstrs -mattr=+load-store-opt,+flat-for-global,-unaligned-access-mode,-dword-access-mode < %s | FileCheck -enable-var-scope -strict-whitespace -check-prefixes=GCN,GFX9,GFX9-ALIGNED %s
+; RUN: llc -march=amdgcn -mcpu=gfx900 -verify-machineinstrs -mattr=+load-store-opt,+flat-for-global,+dword-access-mode < %s | FileCheck -enable-var-scope -strict-whitespace -check-prefixes=GCN,GFX9,GFX9-DWORDALIGNED %s
----------------
arsenm wrote:
> What is -dword-access-mode?
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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87821