This is an archive of the discontinued LLVM Phabricator instance.

TargetMachine: Merge TargetMachine and LLVMTargetMachine
ClosedPublic

Authored by MatzeB on Oct 2 2017, 8:35 PM.

Details

Summary

Merges the TargetMachine and LLVMTargetMachine classes.

  • Merge lib/Target/TargetMachine.cpp and lib/CodeGen/LLVMTargetMachine.cpp into lib/CodeGen/TargetMachine.cpp
  • While at it fix some doxygen comments and remove doxygen comments from implementation when there was already one on the declaration.

See also D38482 for different strategy

Diff Detail

Repository
rL LLVM

Event Timeline

MatzeB created this revision.Oct 2 2017, 8:35 PM
MatzeB added a comment.Oct 2 2017, 9:01 PM

Although not necessarily a measure for code quality; here is:

$ git show --shortstat   # for merging the classes
 51 files changed, 293 insertions(+), 405 deletions(-)
$ git show --shortstat   # for cleaning up the split as in D38482
108 files changed, 273 insertions(+), 230 deletions(-)
echristo accepted this revision.Oct 3 2017, 2:31 PM

Obviously I haven't read the entire patch. I'm in favor of this change though and so will approve it :)

Thanks Matthias!

This revision is now accepted and ready to land.Oct 3 2017, 2:31 PM
This revision was automatically updated to reflect the committed changes.

Turns out this doesn't work nice with the current layering. There are some tools that just link libTarget but not libCodeGen. Merging LLVMTargetMachine and TargetMachine however forces us to move TargetMachine into libCodeGen breaking such users.

I think when merging LLVMTargetMachine and TargetMachine we also have to be consequent and merge libTarget into libCodeGen to facilitate that. Not sure how people think about that, and I'm not sure I will find the time to prepare patches for all llvm users linking to libTarget.

chandlerc edited edge metadata.Oct 12 2017, 6:22 PM

Turns out this doesn't work nice with the current layering. There are some tools that just link libTarget but not libCodeGen. Merging LLVMTargetMachine and TargetMachine however forces us to move TargetMachine into libCodeGen breaking such users.

I think when merging LLVMTargetMachine and TargetMachine we also have to be consequent and merge libTarget into libCodeGen to facilitate that. Not sure how people think about that, and I'm not sure I will find the time to prepare patches for all llvm users linking to libTarget.

FWIW, I think this too would be an improvement. I find the linking of Target vs CodeGen pretty deeply confusing already. But my guess is this would indeed require a bit more work so that layers of the target which really don't require a code generator (and shouldn't) like MC bits can avoid linking with it.

Turns out this doesn't work nice with the current layering. There are some tools that just link libTarget but not libCodeGen. Merging LLVMTargetMachine and TargetMachine however forces us to move TargetMachine into libCodeGen breaking such users.

I think when merging LLVMTargetMachine and TargetMachine we also have to be consequent and merge libTarget into libCodeGen to facilitate that. Not sure how people think about that, and I'm not sure I will find the time to prepare patches for all llvm users linking to libTarget.

FWIW, I think this too would be an improvement. I find the linking of Target vs CodeGen pretty deeply confusing already. But my guess is this would indeed require a bit more work so that layers of the target which really don't require a code generator (and shouldn't) like MC bits can avoid linking with it.

I somewhat agree. I think we'll need to figure out layering between TargetCodeGen (hey, naming is solved) and MC (which is needed by TargetCodeGen), but otherwise I think it works well.

ychen added a subscriber: ychen.Feb 9 2021, 7:43 PM
llvm/trunk/lib/Target/SystemZ/SystemZTargetMachine.cpp