Page MenuHomePhabricator

Change datalayout compatibility check for X86 to allow datalayouts without the new address spaces.
AcceptedPublic

Authored by akhuang on Aug 27 2019, 4:05 PM.

Details

Reviewers
rnk
Summary

Follow up to https://reviews.llvm.org/D64931 to allow existing IR with the old datalayout to compile.

Event Timeline

akhuang created this revision.Aug 27 2019, 4:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 27 2019, 4:05 PM

I suppose would have alleviated the need for most of that datalayout changes in tests?

rnk added a subscriber: echristo.Aug 27 2019, 4:47 PM

I suppose would have alleviated the need for most of that datalayout changes in tests?

Yes and no. I would've still advocated that we do the update and standardize on the new data layout. This just maintains bitcode backwards compatibility.

@echristo reviewed rL243682 / D11654, so I think he should take a look.

rnk accepted this revision.Sep 9 2019, 1:50 PM

lgtm

Sorry this got lost for two weeks. :(

This revision is now accepted and ready to land.Sep 9 2019, 1:50 PM

Still really not a fan of this way. I think you can change the incoming DL in compatibility cases to add an address space when none exists on x86.

-eric

rnk added a comment.Sep 11 2019, 9:23 AM

Still really not a fan of this way. I think you can change the incoming DL in compatibility cases to add an address space when none exists on x86.

-eric

Is that suggestion to handle it in AutoUpgrade.cpp? My rationale for doing it this way was, if a module lacks these address spaces in data layout, it is old bitcode, and it won't use these address spaces for this purpose. But of course, it's unchecked.

This seems to have been causing problems during linking, got hundreds of those:

ld.lld: warning: Linking two modules of different data layouts: '/b/llvm-10/407/lib/libc++.a(cxa_exception.cpp.o at 5739456)' is 'e-m:e-i64:64-f80:128-n8:16:32:64-S128' whereas 'obj/gpr/third_party/protobuf/libprotoc_lib.a(libprotoc_lib.java_message_builder_lite.o at 10565644)' is 'e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128'

If this is only being used for clang-cl (or -fms-compatibility) mode only, is it possible to limit it to just that rather than it affecting pretty much everything?

rnk added a comment.Sep 13 2019, 10:49 AM

If this is only being used for clang-cl (or -fms-compatibility) mode only, is it possible to limit it to just that rather than it affecting pretty much everything?

We decided we'd rather not do that. There needs to be some way to upgrade the x86 datalayout. It can't be frozen in stone forever.

Of course, we need to support the use case of linking old bitcode with new bitcode. Doing that probably requires adding an autoupgrade to replace the old data layout with the new one when loading bitcode. We already have x86-specific intrinsic upgrades in llvm/lib/IR/AutoUpgrade.cpp, so it seems reasonable to add this there, even though it is x86 specific, and remove the X86TargetMachine change.

In D66843#1669803, @rnk wrote:

If this is only being used for clang-cl (or -fms-compatibility) mode only, is it possible to limit it to just that rather than it affecting pretty much everything?

We decided we'd rather not do that. There needs to be some way to upgrade the x86 datalayout. It can't be frozen in stone forever.

Of course, we need to support the use case of linking old bitcode with new bitcode. Doing that probably requires adding an autoupgrade to replace the old data layout with the new one when loading bitcode. We already have x86-specific intrinsic upgrades in llvm/lib/IR/AutoUpgrade.cpp, so it seems reasonable to add this there, even though it is x86 specific, and remove the X86TargetMachine change.

That sounds good; I can work on a patch for that.

I really do think you should just upgrade old layouts to include the address space.

Maybe I'm misunderstanding--is this different from the AutoUpgrade thing that @rnk suggested?