This is an archive of the discontinued LLVM Phabricator instance.

AMDGPU : Fix ISA Version Definitions.
ClosedPublic

Authored by wdng on Jan 10 2017, 1:56 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

wdng updated this revision to Diff 83863.Jan 10 2017, 1:56 PM
wdng retitled this revision from to AMDGPU : Fix ISA Version Definitions..
wdng updated this object.
wdng set the repository for this revision to rL LLVM.
arsenm added inline comments.Jan 10 2017, 2:26 PM
lib/Target/AMDGPU/AMDGPU.td
34–44 ↗(On Diff #83863)

These don't match the terminology used elsewhere. I don't think anything goes slower than quarter rate. Some sources call this 1/16th, but full rate is really 1/4th

wdng added inline comments.Jan 10 2017, 2:37 PM
lib/Target/AMDGPU/AMDGPU.td
34–44 ↗(On Diff #83863)

Do you mean HalfRate64Ops = 1/8 rate and quarter rate = 1/16 rate?

arsenm added inline comments.Jan 10 2017, 2:42 PM
lib/Target/AMDGPU/AMDGPU.td
34–44 ↗(On Diff #83863)

I believe so. The documentation is confusing because it doesn't use a consistent terminology. Full rate is 1/4th of the wave at a time.

wdng added inline comments.Jan 10 2017, 2:51 PM
lib/Target/AMDGPU/AMDGPU.td
34–44 ↗(On Diff #83863)

So any suggestions to name 1/2 rate F64? DoubleRate64Ops?

tony-tye edited reviewers, added: t-tye; removed: tony-tye.Mar 22 2017, 6:08 PM
wdng updated this revision to Diff 101510.Jun 5 2017, 10:30 PM

Add new ISA version definitions & fix incorrect ISA versions.

wdng updated this revision to Diff 101578.Jun 6 2017, 9:20 AM

Add gfx600 and gfx601 definitions.

kzhuravl added inline comments.Jun 6 2017, 9:42 AM
lib/Target/AMDGPU/AMDGPU.td
451 ↗(On Diff #101578)

gfx600/gfx601 are southern islands.

472 ↗(On Diff #101578)

gfx703 is not present in docs update made by @t-tye here: D33736. So either this or the doc need to be updated.

lib/Target/AMDGPU/Processors.td
103–109 ↗(On Diff #101578)

Move these next to other southern islands.

lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp
120 ↗(On Diff #101578)

bad formatting.

wdng updated this revision to Diff 101743.Jun 7 2017, 8:30 AM
wdng marked 3 inline comments as done.

Address code reviews.

t-tye added inline comments.Jun 7 2017, 8:11 PM
lib/Target/AMDGPU/AMDGPU.td
472 ↗(On Diff #101578)

gfx703 was added to docs by D34016.

450 ↗(On Diff #101743)

This is the same as SI/tahiti so I think it should have: FeatureFastFMAF32, HalfRate64Ops

507–516 ↗(On Diff #101743)

Add FeatureGFX9.

lib/Target/AMDGPU/Processors.td
83–93 ↗(On Diff #101743)

SI and tahiti are gfx600 so need to make them have the same attributes and features:

def : ProcessorModel<"gfx600", SIFullSpeedModel,
  [FeatureISAVersion6_0_0]
>;

def : ProcessorModel<"SI", SIFullSpeedModel,
  [FeatureISAVersion6_0_0]
>;

def : ProcessorModel<"tahiti", SIFullSpeedModel,
  [FeatureISAVersion6_0_0]
>;
99–106 ↗(On Diff #101743)

pitcairn/verde/oland/hainan are all gfx601 so suggest:

def : ProcessorModel<"pitcairn", SIQuarterSpeedModel, [FeatureISAVersion6_0_1]>;

def : ProcessorModel<"verde",    SIQuarterSpeedModel, [FeatureISAVersion6_0_1]>;

def : ProcessorModel<"oland",    SIQuarterSpeedModel, [FeatureISAVersion6_0_1]>;

def : ProcessorModel<"hainan",   SIQuarterSpeedModel, [FeatureISAVersion6_0_1]>;
128 ↗(On Diff #101743)

Hawaii is gfx701 so make this FeatureISAVersion7_0_1.

130 ↗(On Diff #101743)

Add:

def : ProcessorModel<"gfx702",     SIQuarterSpeedModel,
  [FeatureISAVersion7_0_1]
>;
132 ↗(On Diff #101743)

kabini is gfx703 so make this FeatureISAVersion7_0_3.

136 ↗(On Diff #101743)

mullins is gfx703 so make this FeatureISAVersion7_0_3.

142–145 ↗(On Diff #101743)

Move this above kabini so in order.

202–216 ↗(On Diff #101743)

The only feature these should have is the corresponding FeatureISAVersion9_0_n as these define the other features.

lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp
148 ↗(On Diff #101743)

Is returning this default the right thing to do? Would it be better to assert since we are supposed to mark all GCN targets with their feature.

wdng updated this revision to Diff 101938.Jun 8 2017, 9:56 AM
wdng marked 12 inline comments as done.

Address code reviews based on Tony's @t-tye comments

t-tye added inline comments.Jun 8 2017, 3:05 PM
lib/Target/AMDGPU/Processors.td
83–93 ↗(On Diff #101743)

I don't think this comment as done as the source is still referring to features, see the suggested test above.

203 ↗(On Diff #101938)

I think we need a GFX9 heading here to match the other sections.

//===----------------------------------------------------------------------===//
// GFX9
//===----------------------------------------------------------------------===//
lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp
107 ↗(On Diff #101938)

To be consistent add period.

// SI.
test/MC/AMDGPU/sym_option.s
39 ↗(On Diff #101938)

I think this should be 1 as hawaii is gfx701.

wdng updated this revision to Diff 102043.Jun 9 2017, 9:21 AM
wdng marked 5 inline comments as done.

Address code reviews.

t-tye accepted this revision.Jun 9 2017, 11:34 AM

LGTM

This revision is now accepted and ready to land.Jun 9 2017, 11:34 AM
wdng updated this revision to Diff 102087.Jun 9 2017, 3:47 PM

Modified lit tests.

t-tye accepted this revision.Jun 9 2017, 4:02 PM

LGTM and address assert issue as a separate patch.

This revision was automatically updated to reflect the committed changes.