This is an archive of the discontinued LLVM Phabricator instance.

Split out `CodeGenTypes` from `CodeGen` for LLT/MVT
ClosedPublic

Authored by chapuni on Apr 19 2023, 5:47 PM.

Details

Summary

This reduces dependencies on llvm-tblgen so much.

CodeGenTypes depends on Support at the moment.
Be careful to append deps on this, since Targets' tablegens
depend on this.

Depends on D149024

Diff Detail

Event Timeline

chapuni created this revision.Apr 19 2023, 5:47 PM
Herald added a project: Restricted Project. · View Herald Transcript
chapuni requested review of this revision.Apr 19 2023, 5:47 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptApr 19 2023, 5:47 PM
chapuni updated this revision to Diff 516182.Apr 23 2023, 9:27 AM
  • Rebase and fixup
chapuni retitled this revision from Split out `CodeGenTypes` from `CodeGen` for LLT/MVT/VT to Split out `CodeGenTypes` from `CodeGen` for LLT/MVT.Apr 23 2023, 9:36 AM
chapuni edited the summary of this revision. (Show Details)
chapuni edited the summary of this revision. (Show Details)Apr 25 2023, 8:18 AM
arsenm accepted this revision.Apr 25 2023, 7:03 PM

I feel like this should have documentation explaining the library split, but not sure where the best place to put that would be

This revision is now accepted and ready to land.Apr 25 2023, 7:03 PM
chapuni added a subscriber: dblaikie.

I am certain this works, though, I would like to ask the 2nd opinion to @dblaikie .

I feel like this should have documentation explaining the library split, but not sure where the best place to put that would be

I think better it may be put in front of add_library(LLVMCodeGenTypes), at the moment.

chapuni updated this revision to Diff 518146.Apr 29 2023, 1:53 AM
  • Add comment lines to LLVMCodeGenTypes
dblaikie accepted this revision.May 1 2023, 4:51 PM

I wouldn't mind a follow-up commit that creates a new/separate directory (llvm/{lib,include/llvm}/CodeGenTypes) but that's probably a lot of unnecessary churn too, so I can appreciate the argument against it - if folks reckon it's better this way, that's OK too. It can just get a bit confusing when the directory/naming doesn't match the layering, etc. Might make it more likely someone regresses these boundaries by introducing new dependencies that break the layering here.

chapuni updated this revision to Diff 518722.May 2 2023, 7:28 AM
chapuni edited the summary of this revision. (Show Details)
  • Updat the desc
This revision was landed with ongoing or failed builds.May 2 2023, 8:18 AM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.May 2 2023, 4:02 PM

I wouldn't mind a follow-up commit that creates a new/separate directory (llvm/{lib,include/llvm}/CodeGenTypes) but that's probably a lot of unnecessary churn too, so I can appreciate the argument against it - if folks reckon it's better this way, that's OK too. It can just get a bit confusing when the directory/naming doesn't match the layering, etc. Might make it more likely someone regresses these boundaries by introducing new dependencies that break the layering here.

I'd really like to see this. That's how virtually all LLVM libraries are laid out, and it removes the PARTIAL_SOURCES_INTENDED kludge.