Page MenuHomePhabricator

Proposed refactoring for lib/Target/X86 to remove layering issue
Needs ReviewPublic

Authored by rtrieu on Apr 2 2019, 6:22 PM.

Details

Summary

Several of the targets in lib/Target have layering issues. Specifically, headers from MCTargetDesc are used when MCTargetDesc is not a dependency. This patch shows the cleanup required for X86 to be fixed of this issue. X86BaseInfo.h is moved to Utils directory, and several enums are moved from X86MCTargetDesc.h into it. This allows InstPrinter to use it without causing a dependency cycle.

The expected dependency list is:
TargetInfo, Utils
InstPrinter
MCTargetDesc
Base Directory, AsmParser, Disassembler

If this is acceptable for X86, I plan on fixing the other targets in a similar manner.

Diff Detail

Event Timeline

rtrieu created this revision.Apr 2 2019, 6:22 PM

I doubt most targets have a Utils library that you can sink a header into. What is the plan for other targets?

rtrieu added a comment.Apr 5 2019, 3:40 PM

I doubt most targets have a Utils library that you can sink a header into. What is the plan for other targets?

A Utils library can be created so that every target will have consistent library/directory structure. That is not much more work than moving a header around.
Alternatively, TargetInfo is the other base library for targets that could home the BaseInfo header.

Why not just merge InstPrinters into MCTargetDesc? It appears Hexagon already does that.

I had assumed that there was some reason to have a library separation between InstPrinter and MCTargetDesc. Merging libraries together would fix the circular dependency, although I would like to know if there is a good reason to keep these two apart.

Other than better Feng Shui, is there a problem you are trying to solve? Is it a building issue?

I think having a project that has a clean dependency graph is a worthwhile goal in itself. The circular dependency here is small, and I'm not aware of a build system that can't handle it, but some do complain about it.
The other goal is consistency of the backends. As Craig mentioned, Hexagon has a small break from the other backends by merging InstPrinter and MCTargetDesc. Having the same library rules across backends would make it less confusing.

bcain added a subscriber: bcain.Apr 9 2019, 5:36 AM
chapuni added a subscriber: chapuni.Apr 9 2019, 1:05 PM

On SystemZ, there is no dependency from InstPrinter to MCTargetDesc (or anything else in the target) as far as I can see. That means we don't have an issue, right?

That's correct, SystemZ doesn't appear to have a dependency problem. Any changes to it will be for maintaining consistency across backends.

https://reviews.llvm.org/D60597

This is following Craig's suggestion to merge InstPrinter into MCTargetDesc, which Hexagon already does. Do people like this solution better?

Yes, D60597 feels better if that helps in this case as well.

rnk added a subscriber: rnk.Apr 19 2019, 2:01 PM

Personally, I wondered why X86MCTargetDesc depends on InstPrinter. I was thinking maybe it would be easiest to cut that dependency edge. Looking at the usages, I think it mostly uses the instprinter do that we can implement various print* methods of MC-derived classes, like X86MCExpr::printImpl. That makes some sense. For LLVM IR, we have a separate AsmParser library, but the AsmWriter is part of IR because it's just too darn convenient to be able to dump anything, any time, code size be damned. So, with that in mind, I guess merging them makes some sense. What do other people think?

The change to merge them is quite large. I wonder if it would be simpler to handle this like we handle clang/Sema+Parse. Aren't those also circularly dependent?

In D60169#1473057, @rnk wrote:

The change to merge them is quite large. I wonder if it would be simpler to handle this like we handle clang/Sema+Parse. Aren't those also circularly dependent?

There is no circular dependency there. Parse depends on Sema, but Sema does not depend on Parse.

rtrieu added a comment.May 7 2019, 9:48 PM

It seems that most people like the change of moving InstPrinter into MCTargetDesc better. If no one has any more comments, I'll start working on patches in that direction.

@rtrieu What's the plan for this particular patch?