This is an archive of the discontinued LLVM Phabricator instance.

AMDGPU: Add sram-ecc feature
ClosedPublic

Authored by kzhuravl on Oct 12 2018, 2:48 PM.

Diff Detail

Event Timeline

kzhuravl created this revision.Oct 12 2018, 2:48 PM

Mostly looks good to me, but I do have some questions / comments.

docs/AMDGPUUsage.rst
263

Maybe better:

Enable/disable compiling code that can run with SRAM ECC enabled.

After all, this option doesn't actually enable the ECC itself, it just makes sure to generate code that works with ECC is enabled. I think we should also mention that code compiled with -msram-ecc will be able to run both with and without ECC.

551

Why not 0x200?

t-tye added inline comments.Oct 16 2018, 9:45 AM
docs/AMDGPUUsage.rst
263

Note that the SRAM ECC setting is being recorded in the code object and that the loader will not load a code object if the settings do not match the target device. This is the same for XNACK. The rational was that by default we don't want the loader to load code objects that will perform sub-optimally. If necessary we could implement options to the loader to ignore checks for developers, but then developers should know what is possible or not by consulting the hardware spec.

kzhuravl updated this revision to Diff 169935.Oct 16 2018, 6:23 PM

Change to 0x200. Minor text updates as suggested by Nicolai.

kzhuravl marked an inline comment as done.Oct 16 2018, 6:24 PM
nhaehnle added inline comments.Oct 17 2018, 3:15 AM
docs/AMDGPUUsage.rst
263

@t-tye, that makes sense.

Thanks for changing the text!

t-tye added inline comments.Oct 22 2018, 1:24 PM
docs/AMDGPUUsage.rst
551

Flag has been updated, but documentation needs corresponding change.

kzhuravl updated this revision to Diff 170487.Oct 22 2018, 1:35 PM

Fix elf flag value in docs.

kzhuravl marked an inline comment as done.Oct 22 2018, 1:36 PM
t-tye added a comment.Nov 5 2018, 9:29 AM

Does the assembler need any support for this? For example, does the .amdgcn_target directive need to accept the +sramecc and ensure the e_flag is set accordingly?

Does scheduler need to check the setting for the instruction latency for 3 operand ALU that involve sub dword operands s a separate patch?

Does the assembler need any support for this? For example, does the .amdgcn_target directive need to accept the +sramecc and ensure the e_flag is set accordingly?

This is already done and tests added. In particular: test/CodeGen/AMDGPU/directive-amdgcn-target.ll

Does scheduler need to check the setting for the instruction latency for 3 operand ALU that involve sub dword operands s a separate patch?

This deserves/will be in separate change.

t-tye accepted this revision.Nov 5 2018, 11:20 AM

LGTM (scheduler can be done in separate change list)

This revision is now accepted and ready to land.Nov 5 2018, 11:20 AM
This revision was automatically updated to reflect the committed changes.