Page MenuHomePhabricator

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

Authored by hakzsam on Mar 28 2018, 2:10 AM.

Details

Reviewers
arsenm
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

Diff Detail

Event Timeline

hakzsam created this revision.Mar 28 2018, 2:10 AM
arsenm added inline comments.Mar 28 2018, 7:32 AM
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 added inline comments.Mar 29 2018, 11:12 AM
test/CodeGen/AMDGPU/load-local-f32.ll
6–9

Where?

Do you want new tests where the option is disabled?

arsenm added inline comments.Mar 29 2018, 11:19 AM
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 updated this revision to Diff 140439.Mar 30 2018, 8:52 AM

v2:

  • use a return ternary operator
  • fix feature format
  • add small test load-local-f32-no-ds128.ll
arsenm accepted this revision.Mar 30 2018, 9:28 AM

LGTM

This revision is now accepted and ready to land.Mar 30 2018, 9:28 AM
jvesely added inline comments.
lib/Target/AMDGPU/AMDGPUSubtarget.h
136

Shouldn't this be initialized in the constructor?

hakzsam added inline comments.Apr 3 2018, 2:30 AM
lib/Target/AMDGPU/AMDGPUSubtarget.h
136

You are right.

hakzsam updated this revision to Diff 140748.Apr 3 2018, 2:33 AM

v3:

  • init EnableDS128 in the constructor
arsenm accepted this revision.Apr 6 2018, 7:52 AM

LGTM