AMDGPU: enable 128-bit for local addr space under an option

[hakzsam]

Summary

ds_read_b128 and ds_write_b128 have been recently enabled
under the amdgpu-ds128 option because the performance benefit
is unclear.

Though, using 128-bit loads/stores for the local address space
appears to introduce regressions in tessellation shaders. Not
sure what is broken, but as ds_read_b128/ds_write_b128 are not
enabled by default, just introduce a global option and enable
128-bit only if requested (until it's fixed/used correctly).

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105464

[arsenm]

lib/Target/AMDGPU/AMDGPU.td

426

Weird formatting/capitalization. "ds_{read|write}_b128 maybe?

lib/Target/AMDGPU/AMDGPUTargetTransformInfo.cpp

273–275

return ternary operator

test/CodeGen/AMDGPU/load-local-f32.ll

6–9

Somewhere needs lines with it disabled

[hakzsam]

test/CodeGen/AMDGPU/load-local-f32.ll

6–9

Where?

Do you want new tests where the option is disabled?

[arsenm]

test/CodeGen/AMDGPU/load-local-f32.ll

6–9

You can either just add another run line and checks, or another small test where it gets used or not. The separate test is probably less effort

[hakzsam]

v2:

• use a return ternary operator
• fix feature format
• add small test load-local-f32-no-ds128.ll

[arsenm]

LGTM

[jvesely]

lib/Target/AMDGPU/AMDGPUSubtarget.h

136

Shouldn't this be initialized in the constructor?

[hakzsam]

lib/Target/AMDGPU/AMDGPUSubtarget.h

136

You are right.

[hakzsam]

v3:

• init EnableDS128 in the constructor

[hakzsam]

ping?

[arsenm]

LGTM

[arsenm]

This was a9a58fa236ab19b5caae32330d31e30ebdf6751f

lib/Target/AMDGPU/AMDGPU.td

   "Enable SI Machine Scheduler"
 >;
 
+def FeatureEnableDS128 : SubtargetFeature<"enable-ds128",
+  "EnableDS128",
+  "true",
+  "Use ds_{read|write}_b128"
+>;
+
 // Unless +-flat-for-global is specified, turn on FlatForGlobal for
 // all OS-es on VI and newer hardware to avoid assertion failures due
 // to missing ADDR64 variants of MUBUF instructions.

lib/Target/AMDGPU/AMDGPUSubtarget.h

   bool EnableLoadStoreOpt;
   bool EnableUnsafeDSOffsetFolding;
   bool EnableSIScheduler;
+  bool EnableDS128;
   bool DumpCode;
 
   // Subtarget statically properties set by tablegen

 
   /// \returns If target supports ds_read/write_b128 and user enables generation
   /// of ds_read/write_b128.
-  bool useDS128(bool UserEnable) const {
-    return CIInsts && UserEnable;
+  bool useDS128() const {
+    return CIInsts && EnableDS128;
   }
 
   /// \returns If MUBUF instructions always perform range checking, even for

lib/Target/AMDGPU/AMDGPUSubtarget.cpp

     EnableLoadStoreOpt(false),
     EnableUnsafeDSOffsetFolding(false),
     EnableSIScheduler(false),
+    EnableDS128(false),
     DumpCode(false),
 
     FP64(false),

lib/Target/AMDGPU/AMDGPUTargetTransformInfo.cpp

     return 512;
   }
 
-  if (AddrSpace == AS.FLAT_ADDRESS ||
-      AddrSpace == AS.LOCAL_ADDRESS ||
-      AddrSpace == AS.REGION_ADDRESS)
+  if (AddrSpace == AS.FLAT_ADDRESS)
     return 128;
 
+  if (AddrSpace == AS.LOCAL_ADDRESS ||
+      AddrSpace == AS.REGION_ADDRESS)
+    return ST->useDS128() ? 128 : 64;
+
   if (AddrSpace == AS.PRIVATE_ADDRESS)
     return 8 * ST->getMaxPrivateElementSize();
 

lib/Target/AMDGPU/SIISelLowering.cpp

   cl::desc("Use GPR indexing mode instead of movrel for vector indexing"),
   cl::init(false));
 
-static cl::opt<bool> EnableDS128(
-  "amdgpu-ds128",
-  cl::desc("Use DS_read/write_b128"),
-  cl::init(false));
-
 static cl::opt<unsigned> AssumeFrameIndexHighZeroBits(
   "amdgpu-frame-index-zero-bits",
   cl::desc("High bits of frame index assumed to be zero"),

     }
   } else if (AS == AMDGPUASI.LOCAL_ADDRESS) {
     // Use ds_read_b128 if possible.
-    if (Subtarget->useDS128(EnableDS128) && Load->getAlignment() >= 16 &&
+    if (Subtarget->useDS128() && Load->getAlignment() >= 16 &&
         MemVT.getStoreSize() == 16)
       return SDValue();
 

     }
   } else if (AS == AMDGPUASI.LOCAL_ADDRESS) {
     // Use ds_write_b128 if possible.
-    if (Subtarget->useDS128(EnableDS128) && Store->getAlignment() >= 16 &&
+    if (Subtarget->useDS128() && Store->getAlignment() >= 16 &&
         VT.getStoreSize() == 16)
       return SDValue();
 

test/CodeGen/AMDGPU/load-local-f32-no-ds128.ll

+; RUN: llc -march=amdgcn -mcpu=tahiti -mattr=-enable-ds128 < %s | FileCheck -check-prefixes=SI,GCN %s
+; RUN: llc -march=amdgcn -mcpu=tonga -mattr=-enable-ds128 < %s | FileCheck -check-prefixes=CIVI,GCN %s
+; RUN: llc -march=amdgcn -mcpu=gfx900 -mattr=-enable-ds128 < %s | FileCheck -check-prefixes=CIVI,GCN %s
+
+; Test if ds_read/write_b128 doesn't gets generated when the option is
+; disabled.
+; GCN-LABEL: {{^}}local_v4f32_to_2b64
+;
+; SI-NOT: ds_read_b128
+; SI-NOT: ds_write_b128
+;
+; CIVI: ds_read2_b64
+; CIVI: ds_write2_b64
+define amdgpu_kernel void @local_v4f32_to_2b64(<4 x float> addrspace(3)* %out, <4 x float> addrspace(3)* %in) {
+  %ld = load <4 x float>, <4 x float> addrspace(3)* %in, align 16
+  store <4 x float> %ld, <4 x float> addrspace(3)* %out, align 16
+  ret void
+}
+
+attributes #0 = { nounwind }

test/CodeGen/AMDGPU/load-local-f32.ll

 ; RUN: llc -march=r600 -mcpu=redwood < %s | FileCheck -check-prefixes=EG,FUNC %s
 
 ; Testing for ds_read/write_128
-; RUN: llc -march=amdgcn -mcpu=tahiti -amdgpu-ds128 < %s | FileCheck -check-prefixes=SI,FUNC %s
-; RUN: llc -march=amdgcn -mcpu=tonga -amdgpu-ds128 < %s | FileCheck -check-prefixes=CIVI,FUNC %s
-; RUN: llc -march=amdgcn -mcpu=gfx900 -amdgpu-ds128 < %s | FileCheck -check-prefixes=CIVI,FUNC %s
+; RUN: llc -march=amdgcn -mcpu=tahiti -mattr=+enable-ds128 < %s | FileCheck -check-prefixes=SI,FUNC %s
+; RUN: llc -march=amdgcn -mcpu=tonga -mattr=+enable-ds128 < %s | FileCheck -check-prefixes=CIVI,FUNC %s
+; RUN: llc -march=amdgcn -mcpu=gfx900 -mattr=+enable-ds128 < %s | FileCheck -check-prefixes=CIVI,FUNC %s
 
 ; FUNC-LABEL: {{^}}load_f32_local:
 ; SICIVI: s_mov_b32 m0

test/CodeGen/AMDGPU/load-local-f64.ll

 ; RUN: llc -march=r600 -mcpu=redwood < %s | FileCheck -check-prefixes=EG,FUNC %s
 
 ; Testing for ds_read_b128
-; RUN: llc -march=amdgcn -mcpu=tonga -verify-machineinstrs -amdgpu-ds128 < %s | FileCheck -check-prefixes=CIVI,FUNC %s
-; RUN: llc -march=amdgcn -mcpu=gfx900 -verify-machineinstrs -amdgpu-ds128 < %s | FileCheck -check-prefixes=CIVI,FUNC %s
+; RUN: llc -march=amdgcn -mcpu=tonga -verify-machineinstrs -mattr=+enable-ds128 < %s | FileCheck -check-prefixes=CIVI,FUNC %s
+; RUN: llc -march=amdgcn -mcpu=gfx900 -verify-machineinstrs -mattr=+enable-ds128 < %s | FileCheck -check-prefixes=CIVI,FUNC %s
 
 ; FUNC-LABEL: {{^}}local_load_f64:
 ; SICIV: s_mov_b32 m0

test/CodeGen/AMDGPU/load-local-i16.ll

 ; RUN: llc -march=r600 -mcpu=redwood -verify-machineinstrs < %s | FileCheck -check-prefix=EG -check-prefix=FUNC %s
 
 ; Testing for ds_read/write_b128
-; RUN: llc -march=amdgcn -mcpu=tonga -amdgpu-ds128 < %s | FileCheck -check-prefixes=CIVI,FUNC %s
-; RUN: llc -march=amdgcn -mcpu=gfx900 -amdgpu-ds128 < %s | FileCheck -check-prefixes=CIVI,FUNC %s
+; RUN: llc -march=amdgcn -mcpu=tonga -mattr=+enable-ds128 < %s | FileCheck -check-prefixes=CIVI,FUNC %s
+; RUN: llc -march=amdgcn -mcpu=gfx900 -mattr=+enable-ds128 < %s | FileCheck -check-prefixes=CIVI,FUNC %s
 
 ; FUNC-LABEL: {{^}}local_load_i16:
 ; GFX9-NOT: m0

test/CodeGen/AMDGPU/load-local-i32.ll

 ; RUN: llc -march=r600 -mcpu=redwood < %s | FileCheck -check-prefix=EG -check-prefix=FUNC %s
 
 ; Testing for ds_read/write_128
-; RUN: llc -march=amdgcn -mcpu=tahiti -amdgpu-ds128 < %s | FileCheck -check-prefixes=SI,FUNC %s
-; RUN: llc -march=amdgcn -mcpu=tonga -amdgpu-ds128 < %s | FileCheck -check-prefixes=CIVI,FUNC %s
-; RUN: llc -march=amdgcn -mcpu=gfx900 -amdgpu-ds128 < %s | FileCheck -check-prefixes=CIVI,FUNC %s
+; RUN: llc -march=amdgcn -mcpu=tahiti -mattr=+enable-ds128 < %s | FileCheck -check-prefixes=SI,FUNC %s
+; RUN: llc -march=amdgcn -mcpu=tonga -mattr=+enable-ds128 < %s | FileCheck -check-prefixes=CIVI,FUNC %s
+; RUN: llc -march=amdgcn -mcpu=gfx900 -mattr=+enable-ds128 < %s | FileCheck -check-prefixes=CIVI,FUNC %s
 
 ; FUNC-LABEL: {{^}}local_load_i32:
 ; GCN-NOT: s_wqm_b64

test/CodeGen/AMDGPU/load-local-i64.ll

 ; RUN: llc -march=r600 -mcpu=redwood < %s | FileCheck -check-prefixes=EG,FUNC %s
 
 ; Testing for ds_read/write_b128
-; RUN: llc -march=amdgcn -mcpu=tonga -verify-machineinstrs -amdgpu-ds128 < %s | FileCheck -check-prefixes=CIVI,FUNC %s
-; RUN: llc -march=amdgcn -mcpu=gfx900 -verify-machineinstrs -amdgpu-ds128 < %s | FileCheck -check-prefixes=CIVI,FUNC %s
+; RUN: llc -march=amdgcn -mcpu=tonga -verify-machineinstrs -mattr=+enable-ds128 < %s | FileCheck -check-prefixes=CIVI,FUNC %s
+; RUN: llc -march=amdgcn -mcpu=gfx900 -verify-machineinstrs -mattr=+enable-ds128 < %s | FileCheck -check-prefixes=CIVI,FUNC %s
 
 ; FUNC-LABEL: {{^}}local_load_i64:
 ; SICIVI: s_mov_b32 m0

test/CodeGen/AMDGPU/load-local-i8.ll

 ; RUN: llc -march=r600 -mtriple=r600---amdgiz -mcpu=redwood -verify-machineinstrs < %s | FileCheck -check-prefix=EG -check-prefix=FUNC %s
 
 ; Testing for ds_read/write_b128
-; RUN: llc -march=amdgcn -mcpu=tonga -amdgpu-ds128 < %s | FileCheck -check-prefixes=CIVI,FUNC %s
-; RUN: llc -march=amdgcn -mcpu=gfx900 -amdgpu-ds128 < %s | FileCheck -check-prefixes=CIVI,FUNC %s
+; RUN: llc -march=amdgcn -mcpu=tonga -mattr=+enable-ds128 < %s | FileCheck -check-prefixes=CIVI,FUNC %s
+; RUN: llc -march=amdgcn -mcpu=gfx900 -mattr=+enable-ds128 < %s | FileCheck -check-prefixes=CIVI,FUNC %s
 
 ; FUNC-LABEL: {{^}}local_load_i8:
 ; GCN-NOT: s_wqm_b64