Page MenuHomePhabricator

[HIP] Support gpu arch gfx906+sram-ecc
AbandonedPublic

Authored by yaxunl on Mar 26 2019, 7:51 PM.

Details

Reviewers
tra
ashi1
arsenm
Summary

Add a new gpu arch gfx906+sram-ecc for HIP.

It is similar to gfx906 but implies target feature sram-ecc enabled,
whereas gfx906 implies target sram-ecc disabled. Corresponding
option -mattr=[+|-]sram-ecc will be passed to llc based on the
gpu arch.

Diff Detail

Event Timeline

yaxunl created this revision.Mar 26 2019, 7:51 PM
tra added inline comments.Mar 27 2019, 10:39 AM
lib/Basic/Cuda.cpp
113

Wording nit:
Does it mean +(SRAM, ECC) or +SRAM, -ECC ?

From the rest of the changes I guess it's the former, but the syntax used can easily be interpreted as the later.

Perhaps change the feature name to sram_ecc and GPU name to something less verbose. gfx906se ?

yaxunl marked an inline comment as done.Mar 27 2019, 11:05 AM
yaxunl added inline comments.
lib/Basic/Cuda.cpp
113

It actually means +(SRAM-ECC).

There is actually documentation about what features are supported and their format

https://llvm.org/docs/AMDGPUUsage.html#code-object-target-identification
https://llvm.org/docs/AMDGPUUsage.html#amdgpu-target-features

Since the name has been used in runtime, we cannot change that.

Not all combinations are useful, therefore this patch only supports the one that is known to be useful.

I am considering a more generic approach, but so far I do not see its necessity yet.

arsenm requested changes to this revision.Mar 27 2019, 1:24 PM
arsenm added a subscriber: arsenm.

This should not be a new device name. This is also not how the features should be passed to the backend. These should be added to the function IR directly

This revision now requires changes to proceed.Mar 27 2019, 1:24 PM
yaxunl abandoned this revision.Thu, Jun 13, 12:16 PM