This is an archive of the discontinued LLVM Phabricator instance.

AMDGPU: Remove #include "MCTargetDesc/AMDGPUMCTargetDesc.h" from common headers
ClosedPublic

Authored by tstellar on Apr 30 2018, 10:05 AM.

Details

Summary

MCTargetDesc/AMDGPUMCTargetDesc.h contains enums for all the instuction
and register defintions, which are huge so we only want to include
them where needed.

This will also make it easier if we want to split the R600 and GCN
definitions into separate tablegenerated files.

Diff Detail

Repository
rL LLVM

Event Timeline

tstellar created this revision.Apr 30 2018, 10:05 AM
nhaehnle accepted this revision.May 3 2018, 2:56 AM

There are some unrelated changes, which I think should be mentioned in the commit description. Apart from that LGTM.

lib/Target/AMDGPU/AMDGPURegisterInfo.cpp
17 ↗(On Diff #144580)

Why is this necessary? Shouldn't headers generally be self-contained (i.e. include all their dependencies)?

lib/Target/AMDGPU/AMDGPUSubtarget.h
26 ↗(On Diff #144580)

Ah I see. This seems like a separate change, although I wouldn't mind committing them together as long as the description is updated to reflect this.

This revision is now accepted and ready to land.May 3 2018, 2:56 AM
tstellar added inline comments.May 3 2018, 8:32 AM
lib/Target/AMDGPU/AMDGPUSubtarget.h
26 ↗(On Diff #144580)

These are both related changes. I was unable to remove AMDGPUMCTargetDesc.h from SIMachineFunctionInfo.h because it uses some enums from the header to initialize default values for the SIMachineFunction class. Having AMDGPUSubtarget.h include SIMachineFunctionInfo.h would then pull in AMDGPUMCTargetDesc.h which would defeat the purpose of the patch since AMDGPUSubtarget.h is included everywhere.

This was a compromise to keep the patch simple and avoid rewriting the SIMachineFunctionInfo class.

This revision was automatically updated to reflect the committed changes.