This is an archive of the discontinued LLVM Phabricator instance.

AMDGPU: Refactor Subtarget classes
ClosedPublic

Authored by tstellar on Jul 6 2018, 11:25 AM.

Details

Summary

This is a follow-up to r335942.

  • Merge SISubtarget into AMDGPUSubtarget and rename to GCNSubtarget
  • Rename AMDGPUCommonSubtarget to AMDGPUSubtarget

Diff Detail

Repository
rL LLVM

Event Timeline

tstellar created this revision.Jul 6 2018, 11:25 AM
arsenm accepted this revision.Jul 6 2018, 11:35 AM

LGTM

This revision is now accepted and ready to land.Jul 6 2018, 11:35 AM
jvesely accepted this revision.Jul 6 2018, 12:37 PM

Other than the few nits mentioned in the text, LGTM.

lib/Target/AMDGPU/AMDGPUInstructionSelector.h
42 ↗(On Diff #154439)

This rename duplicates GCNSubtarget declaration.

lib/Target/AMDGPU/AMDGPUSubtarget.h
230 ↗(On Diff #154439)

Just out of curiosity what's the benefit of having this enum split between GCNSUbtarget and R600Subtarget, vs merging it in AMDGPUSubtarget?

981 ↗(On Diff #154439)

Is this #if 0 code here intentionally? If yes an explanatory comment would be nice.

lib/Target/AMDGPU/R600RegisterInfo.h
23 ↗(On Diff #154439)

Why is this necessary in R600 file? Looks like a leftover from the time when AMDGPUSubtarget was shared parent.

lib/Target/AMDGPU/SIRegisterInfo.h
27 ↗(On Diff #154439)

This rename duplicates GCNSUbtarget declaration.

tstellar marked 5 inline comments as done.Jul 11 2018, 2:06 PM
tstellar added inline comments.
lib/Target/AMDGPU/AMDGPUSubtarget.h
230 ↗(On Diff #154439)

There is no benefit, so I've merged the enums back together.

This revision was automatically updated to reflect the committed changes.
tstellar marked an inline comment as done.