[PATCH] D30047: [GlobalISel] Split LowLevelType to a fundamental type in libSupport.

[Daniel Sanders via Phabricator via llvm-commits]

dsanders added a comment.

In https://reviews.llvm.org/D30047#682181, @rovka wrote:

> In https://reviews.llvm.org/D30047#679944, @dsanders wrote:
>
> > 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.


https://reviews.llvm.org/D30047