This is an archive of the discontinued LLVM Phabricator instance.

[globalisel] Change LLT constructor string into an LLT-based object that knows how to generate it.
ClosedPublic

Authored by dsanders on Feb 16 2017, 8:04 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

dsanders created this revision.Feb 16 2017, 8:04 AM
ab requested changes to this revision.Feb 16 2017, 9:20 AM

This is a layering violation, and that's a big no-no. For instance, it woudln't be too hard to break the tblgen link by doing NFC changes to LLT.

I think the least bad option is to move the self-contained parts of LLT to some llvm/Support/LowLevelType.h, and put the Type* <> LLT conversions in llvm/CodeGen/LowLevelType.h, which would just include llvm/Support/LowLevelType.h.

And MVT isn't a good example to follow, in part because it's header-only, IIRC completely self-contained, and IIRC doesn't reference IR Type at all.

This revision now requires changes to proceed.Feb 16 2017, 9:20 AM
In D30046#678788, @ab wrote:

This is a layering violation, and that's a big no-no. For instance, it woudln't be too hard to break the tblgen link by doing NFC changes to LLT.

I think the least bad option is to move the self-contained parts of LLT to some llvm/Support/LowLevelType.h, and put the Type* <> LLT conversions in llvm/CodeGen/LowLevelType.h, which would just include llvm/Support/LowLevelType.h.

And MVT isn't a good example to follow, in part because it's header-only, IIRC completely self-contained, and IIRC doesn't reference IR Type at all.

The main difficulty I'm having with splitting it is functions like:

static LLT scalar(unsigned SizeInBits);

and other functions that return LLT. I need them to exist in both lib/CodeGen and lib/Support but I can't distribute the class declaration between the two.

The main difficulty I'm having with splitting it is functions like:

static LLT scalar(unsigned SizeInBits);

and other functions that return LLT. I need them to exist in both lib/CodeGen and lib/Support but I can't distribute the class declaration between the two.

Huh, I probably poorly explained my thought. Here's an attempt: D30047

Note that it ends up including CodeGen/MVT from libSupport, which in theory is also gross, but in practice should never become a problem. We might want to consider fixing that too though.

dsanders updated this revision to Diff 89525.Feb 23 2017, 9:53 AM
dsanders edited edge metadata.

Update following discussion in D30047

For the header, I went with Support/LowLevelTypeImpl.h on the basis that more people develop CodeGen than TableGen.

dsanders edited the summary of this revision. (Show Details)Feb 24 2017, 8:02 AM

We seem to have reached an agreement on D30047 so I intend to commit this soon.

dsanders edited the summary of this revision. (Show Details)Feb 24 2017, 8:05 AM
dsanders updated this revision to Diff 90003.Feb 28 2017, 3:27 AM

Refresh before commit

This revision was automatically updated to reflect the committed changes.
dsanders reopened this revision.Mar 2 2017, 6:56 AM
dsanders updated this revision to Diff 90458.Mar 3 2017, 4:29 AM

Bring back the original intent of the patch which went missing in diff 89525.

Update with the modulemap solution. The constructor solution is also viable but
when I brought back the missing code I noticed that MVTToLLT() ought to be
implemented with that constructor once we can import all SelectionDAG types
and that means we'll have to bring it back to Support by then.

Tested with module support enabled.

dsanders updated this revision to Diff 90459.Mar 3 2017, 4:34 AM

Fix typo in (currently unused) tablegen output: LLT:vector -> LLT::vector

dsanders retitled this revision from [globalisel] Change LLT constructor string into an LLT subclass that knows how to generate it. to [globalisel] Change LLT constructor string into an LLT-based object that knows how to generate it..Mar 7 2017, 10:43 AM
This revision was automatically updated to reflect the committed changes.