Change X86 datalayout for three address spaces that specify pointer sizes.

[akhuang]

Summary

This patch adds -p270:32:32-p271:32:32-p272:64:64 to the X86 data layout string, to be used for implementing mixed pointer sizes.

This will be used for an upcoming change that uses these three address spaces which will be used to implement the microsoft extensions ptr32, ptr64, sptr, and uptr. The address spaces specify whether a pointer is 32 bit sign extended, 32 bit zero extended, or 64 bit.

This patch also changes the datalayout string in tests that contain a datalayout string, because the datalayout specified in the llvm module has to match the target datalayout.

The numbers 270-272 are more or less arbitrary; I picked them because they're near 256-258, which are the current existing address spaces.

[rnk]

I suggested to @akhuang offline that we should discuss this on llvm-dev. I'll add some other X86 reviewers.

[akhuang]

The llvm-dev discussion is here http://lists.llvm.org/pipermail/llvm-dev/2019-July/134035.html
I think the consensus is that it should be fine to change the data layout.

[lebedev.ri]

Why do you need to change (update) the datalayout in every single test?
That looks extremely noisy and hides the actually-needed changes.

[lebedev.ri]

clang/lib/Basic/Targets/OSTargets.h

778–780

I'd expect that this should be guarded by whatever flag is used for ms extensions.
Put differently, i i'm not sure that when those extensions are not enabled, the datalayout should be changed?

[rnk]

clang/lib/Basic/Targets/OSTargets.h

778–780

As discussed in the RFC, other people find these address spaces generally useful, and there is no need to limit them to just Windows or *-windows-msvc targets. Making it conditional would require mirroring the same conditional into LLVM, because LLVM and clang have to agree on data layout.

[akhuang]

For some reason the tests were failing before without the datalayout change? I'm not sure why, but I changed them back and they're fine.

[akhuang]

Remove test case changes.

[lebedev.ri]

llvm/lib/Target/X86/X86TargetMachine.cpp

120–121

I may be wrong but this seems to not match what has been said in the responses to RFC.

[akhuang]

@lebedev.ri The test case datalayout strings were changed because somewhere llvm asserts that the string in the IR matches the backend datalayout. I don't know why I wasn't getting the assert error now, but I think they'll all have to be changed if we change the X86 datalayout?

[lebedev.ri]

In D64931#1622038, @akhuang wrote:

@lebedev.ri The test case datalayout strings were changed because somewhere llvm asserts that the string in the IR matches the backend datalayout. I don't know why I wasn't getting the assert error now, but I think they'll all have to be changed

Can you post a reproducer?

In D64931#1622038, @akhuang wrote:

if we change the X86 datalayout?

I think this is precisely what was discussed in replies to RFC - this hardcodes these address spaces,
and thus either makes them unavaliable for other front-ends and/or forces them to use them with Precisely Same Meaning.

[akhuang]

Can you post a reproducer?

Turns out I just didn't have assertions enabled. With assertions the changed test cases should fail.

I think this is precisely what was discussed in replies to RFC - this hardcodes these address spaces, and thus either makes them unavaliable for other front-ends and/or forces them to use them with Precisely Same Meaning.

It seems like the RFC replies agreed that this should be implemented with address spaces, and that the information for these is encoded in the data layout. I think there was some confusion as to whether there would be more changes in addition to the data layout change?

[lebedev.ri]

In D64931#1622144, @akhuang wrote:

Can you post a reproducer?

Turns out I just didn't have assertions enabled. With assertions the changed test cases should fail.

I think this is precisely what was discussed in replies to RFC - this hardcodes these address spaces, and thus either makes them unavaliable for other front-ends and/or forces them to use them with Precisely Same Meaning.

It seems like the RFC replies agreed that this should be implemented with address spaces, and that the information for these is encoded in the data layout.

Yes, *in front-end*.
llvm/lib/Target/X86/X86TargetMachine.cpp is *not* front-end, therefore

In D64931#1622039, @lebedev.ri wrote:

this hardcodes these address spaces, and thus either makes them unavaliable for other front-ends and/or forces them to use them with Precisely Same Meaning.

In D64931#1622144, @akhuang wrote:

I think there was some confusion as to whether there would be more changes in addition to the data layout change?

[arsenm]

In D64931#1622039, @lebedev.ri wrote:

In D64931#1622038, @akhuang wrote:

@lebedev.ri The test case datalayout strings were changed because somewhere llvm asserts that the string in the IR matches the backend datalayout. I don't know why I wasn't getting the assert error now, but I think they'll all have to be changed

Can you post a reproducer?

In D64931#1622038, @akhuang wrote:

if we change the X86 datalayout?

I think this is precisely what was discussed in replies to RFC - this hardcodes these address spaces,
and thus either makes them unavaliable for other front-ends and/or forces them to use them with Precisely Same Meaning.

Address space have backend defined semantics, and aren’t really reserved for front end use. I think the fact that non-0 address spaces on X86 codegen the same as address space 0 and could be used for something by a front end is an accident of how SelectionDAG is implemented. If X86 wants to reserve address space ranges for frontend use, that would need to be decided and documented. You don’t necessarily get the current behavior for free in GlobalISel since pointer types are distinct, so this would specifically need to be implemented.

[akhuang]

• Change the datalayout strings in test cases for x86 so they match the new datalayout and tests pass.
• Change the address space numbers, mostly so I don't have to change the fact that currently address spaces under

256 don't do anything.

[akhuang]

Address space have backend defined semantics, and aren’t really reserved for front end use. I think the fact that non-0 address spaces on X86 codegen the same as address space 0 and could be used for something by a front end is an accident of how SelectionDAG is implemented. If X86 wants to reserve address space ranges for frontend use, that would need to be decided and documented. You don’t necessarily get the current behavior for free in GlobalISel since pointer types are distinct, so this would specifically need to be implemented.

By this do you mean that this would be an instance of address spaces being used by the frontend? Or just that adding meaning to address spaces shouldn't be breaking other frontends?

[arsenm]

In D64931#1633669, @akhuang wrote:

Address space have backend defined semantics, and aren’t really reserved for front end use. I think the fact that non-0 address spaces on X86 codegen the same as address space 0 and could be used for something by a front end is an accident of how SelectionDAG is implemented. If X86 wants to reserve address space ranges for frontend use, that would need to be decided and documented. You don’t necessarily get the current behavior for free in GlobalISel since pointer types are distinct, so this would specifically need to be implemented.

By this do you mean that this would be an instance of address spaces being used by the frontend? Or just that adding meaning to address spaces shouldn't be breaking other frontends?

I mean if frontends are relying on current specific behavior for these address spaces, that’s not something that’s documented to work. It just happens to

[akhuang]

• Change datalayout in lld test cases.

[rnk]

I think we're ready to proceed here, lgtm. Shout if I've misrepresented anything. :)

[akhuang]

Pinging reviewers -- are there any other concerns on this patch?

[akhuang]

Rebase

[akhuang]

Commited in r370083

[dyung]

Hi, this is causing 3 test failures on the PS4 linux bot. The changes may not have been initially flagged because a different issue was causing a build failure which masked the problem. I have bisected the test failures to this change though.

http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/builds/53916

LLVM :: LTO/Resolution/X86/not-prevailing-weak-aliasee.ll
LLVM :: ThinLTO/X86/printer.ll
lld :: ELF/lto/drop-debug-info.ll

Can you take a look?

[dyung]

In D64931#1647880, @dyung wrote:

Hi, this is causing 3 test failures on the PS4 linux bot. The changes may not have been initially flagged because a different issue was causing a build failure which masked the problem. I have bisected the test failures to this change though.

http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/builds/53916

LLVM :: LTO/Resolution/X86/not-prevailing-weak-aliasee.ll
LLVM :: ThinLTO/X86/printer.ll
lld :: ELF/lto/drop-debug-info.ll

Actually it appears the llvm tests were fixed up in r370105 leaving only the LLD test failing.

Can you take a look?