This is an archive of the discontinued LLVM Phabricator instance.

[GlobalISel] Split LowLevelType to a fundamental type in libSupport.
AbandonedPublic

Authored by ab on Feb 16 2017, 10:09 AM.

Details

Summary

This is an attempt at splitting LLT into a libSupport component, usable from tblgen, as discussed in D30046.

It's somewhat awkward (and ends up including MVT, which also should have been in libSupport), but it's the best solution I found so far.

WDYT?

Diff Detail

Event Timeline

ab created this revision.Feb 16 2017, 10:09 AM
dsanders edited edge metadata.Feb 17 2017, 7:59 AM

I've been trying to preserve the current style of LLT usage but having seen this I realize there's no need to do that.

I think we should make llvm::getLLT() a member of Type (see example usage below and https://reviews.llvm.org/differential/diff/88897/) but aside from that and a couple nits I think this seems like a reasonable way to fix the layering problem.

I'm inclined to leave the MVT issue as-is since that's at least a header-only class.

include/llvm/CodeGen/LowLevelType.h
24–27

LLVMContext, and raw_ostream aren't needed anymore.

lib/Target/AArch64/AArch64CallLowering.cpp
195

For example:

SplitTy->getLLT(DL)
rovka edited edge metadata.Feb 21 2017, 6:43 AM

I've been trying to preserve the current style of LLT usage but having seen this I realize there's no need to do that.

I think we should make llvm::getLLT() a member of Type (see example usage below and https://reviews.llvm.org/differential/diff/88897/) but aside from that and a couple nits I think this seems like a reasonable way to fix the layering problem.

Not sure if this has been decided yet or not, but here's my 2 cents anyway:

I find it a bit weird to have IR level types know about low level types, we usually have it the other way around (e.g. EVT::getTypeForEVT). I prefer a standalone method as defined here, but OTOH having a LowLevelTypes.h in both CodeGen and Support might be confusing. How about we move it to GlobalISel/Utils.h? Or think of a different name for it (FWIW clang uses CodeGenTypes.h for the correspondence between AST types and LLVM types).

I've been trying to preserve the current style of LLT usage but having seen this I realize there's no need to do that.

I think we should make llvm::getLLT() a member of Type (see example usage below and https://reviews.llvm.org/differential/diff/88897/) but aside from that and a couple nits I think this seems like a reasonable way to fix the layering problem.

Not sure if this has been decided yet or not, but here's my 2 cents anyway:

I don't think we've reached a conclusion yet.

I find it a bit weird to have IR level types know about low level types, we usually have it the other way around (e.g. EVT::getTypeForEVT).

EVT can do it that way around because libLLVMCodeGen depends on libLLVMCore. If we move LLT to libLLVMSupport then we can't depend on Type from libLLVMCore (because libLLVMCore depends on libLLVMSupport). We could declare the function in libLLVMSupport and implement it in libLLVMCodeGen but I'm not sure that's an improvement on just having tablegen include the header from libLLVMCodeGen.

I prefer a standalone method as defined here, but OTOH having a LowLevelTypes.h in both CodeGen and Support might be confusing. How about we move it to GlobalISel/Utils.h? Or think of a different name for it (FWIW clang uses CodeGenTypes.h for the correspondence between AST types and LLVM types).

Ok, let's go with the standalone method. I'd like to give it a more descriptive name though seeing as it's in the llvm namespace. How about getLLTForType()?

For the header name, I agree with not having LowLevelTypes.h in both libraries. Let's go with GlobalISel/Utils.h.

rovka added a comment.Feb 22 2017, 1:05 AM

Ok, let's go with the standalone method. I'd like to give it a more descriptive name though seeing as it's in the llvm namespace. How about getLLTForType()?

For the header name, I agree with not having LowLevelTypes.h in both libraries. Let's go with GlobalISel/Utils.h.

That sounds good to me. @ab, what do you think?

ab added a comment.Feb 22 2017, 10:07 AM

Ok, let's go with the standalone method. I'd like to give it a more descriptive name though seeing as it's in the llvm namespace. How about getLLTForType()?

getLLTForType SGTM

For the header name, I agree with not having LowLevelTypes.h in both libraries. Let's go with GlobalISel/Utils.h.

That sounds good to me. @ab, what do you think?

Yeah, using the same name is weird, but here's the rationale: almost every single user of LLT should use CodeGen/LowLevelTypes.h; for them, that's the way to use LLT. But I guess that illusion breaks down pretty quickly when they find the header almost completely empty... Maybe we can rename Support/LowLevelTypes.h Support/LowLevelTypeImpl.h or something? Not that it sounds much better ;)

But specifically regarding Utils.h: I'm not a big fan of catch-all headers; maybe we can keep it single-purpose and call it CodeGen/LowLevelTypeUtils.h, or CodeGen/CodeGenLowLevelTypes.h, or something like that?

Not a big deal either way; if we don't have a better idea I'll stick to Utils.h.

Unfortunately, this approach introduces a circular dependency when LLVM_ENABLE_MODULES=1. The failure was in http://lab.llvm.org:8011/builders/clang-x86_64-linux-selfhost-modules-2/builds/4942 and was caused by the following dependency loop:

LLVM_Utils -> LLVM_CodeGen_MachineValueType -> LLVM_Utils

To write it another way, we have these dependencies:

  • TableGen -> Support
  • CodeGen -> Support
  • Support -> CodeGen

I can see two ways forward:

  • We use the original version of D30046 (https://reviews.llvm.org/D30046?id=88737) so that we continue to have the following dependencies:
    • TableGen -> Support
    • TableGen -> CodeGen
    • CodeGen -> Support
  • We also move the MVT type to Support so we remove the TableGen->CodeGen dependency and are left with:
    • TableGen -> Support
    • CodeGen -> Support

Which would you prefer?

ab added a comment.Feb 28 2017, 7:04 PM

Unfortunately, this approach introduces a circular dependency when LLVM_ENABLE_MODULES=1. The failure was in http://lab.llvm.org:8011/builders/clang-x86_64-linux-selfhost-modules-2/builds/4942 and was caused by the following dependency loop:

LLVM_Utils -> LLVM_CodeGen_MachineValueType -> LLVM_Utils

Oh boy..

To write it another way, we have these dependencies:

  • TableGen -> Support
  • CodeGen -> Support
  • Support -> CodeGen

None of these are new, right? Looking at the log, none of the failures are related to LLT.

I can see two ways forward:

  • We use the original version of D30046 (https://reviews.llvm.org/D30046?id=88737) so that we continue to have the following dependencies:
    • TableGen -> Support
    • TableGen -> CodeGen
    • CodeGen -> Support
  • We also move the MVT type to Support so we remove the TableGen->CodeGen dependency and are left with:
    • TableGen -> Support
    • CodeGen -> Support

How about a lazy variant of this: move MVT to the Utils module, without changing its path on disk. It's already in its own special little module anyway.

Honestly, I'd love for us to finally tackle MVT, but right in the middle of branches is going to add a lot of merge pain..

In D30047#689257, @ab wrote:

Unfortunately, this approach introduces a circular dependency when LLVM_ENABLE_MODULES=1. The failure was in http://lab.llvm.org:8011/builders/clang-x86_64-linux-selfhost-modules-2/builds/4942 and was caused by the following dependency loop:

LLVM_Utils -> LLVM_CodeGen_MachineValueType -> LLVM_Utils

Oh boy..

To write it another way, we have these dependencies:

  • TableGen -> Support
  • CodeGen -> Support
  • Support -> CodeGen

None of these are new, right?

Support -> CodeGen is new. It's the include of MachineValueType.h in LowLevelTypeImpl.h that introduces it.

Before this patch the dependencies were:

  • TableGen -> Support
  • TableGen -> CodeGen
  • CodeGen -> Support

and this patch changed 'TableGen -> CodeGen' into 'TableGen -> Support -> CodeGen'.

Looking at the log, none of the failures are related to LLT.

LLT is mentioned in the 'While building ...' lines:

While building module 'LLVM_Utils' imported from /home/buildbot/modules-slave-2/clang-x86_64-linux-selfhost-modules-2/llvm.src/lib/TableGen/StringMatcher.cpp:14:
While building module 'LLVM_CodeGen_MachineValueType' imported from /home/buildbot/modules-slave-2/clang-x86_64-linux-selfhost-modules-2/llvm.src/include/llvm/Support/LowLevelTypeImpl.h:32:
In file included from <module-includes>:1:
/home/buildbot/modules-slave-2/clang-x86_64-linux-selfhost-modules-2/llvm.src/include/llvm/CodeGen/MachineValueType.h:18:10: fatal error: cyclic dependency in module 'LLVM_Utils': LLVM_Utils -> LLVM_CodeGen_MachineValueType -> LLVM_Utils

I can see two ways forward:

  • We use the original version of D30046 (https://reviews.llvm.org/D30046?id=88737) so that we continue to have the following dependencies:
    • TableGen -> Support
    • TableGen -> CodeGen
    • CodeGen -> Support
  • We also move the MVT type to Support so we remove the TableGen->CodeGen dependency and are left with:
    • TableGen -> Support
    • CodeGen -> Support

How about a lazy variant of this: move MVT to the Utils module, without changing its path on disk. It's already in its own special little module anyway.

I'll give it a go. I'll also try removing the MVT dependency since the relevant constructor isn't used very often as far as I can tell. We might be able to switch it for something like getLLTForType().

ab added a comment.Mar 2 2017, 10:54 AM

Ah, you're right, I completely forgot about the MVT ctor. Yeah, it's probably better to add a getLLTForMVT() in the CodeGen header.

ab added a comment.Mar 2 2017, 10:58 AM

By the way, I think you kept the commit message of the original patch in r296474. It's somewhat confusing; I don't even remember what it meant ;)

In D30047#690852, @ab wrote:

Ah, you're right, I completely forgot about the MVT ctor. Yeah, it's probably better to add a getLLTForMVT() in the CodeGen header.

I've ended up going with the modulemap approach since TableGen's MVTToLLT() should eventually use that constructor.

In D30047#690854, @ab wrote:

By the way, I think you kept the commit message of the original patch in r296474. It's somewhat confusing; I don't even remember what it meant ;)

MVTToLLT() used to convert MVT's into strings containing calls to the LLT constructor. It now converts MVT's into LLTCodeGen objects which know how to build that string from an LLT and also allows tablegen to inspect the LLT.

ab abandoned this revision.Mar 23 2017, 1:10 PM

Daniel committed this in rL297177.