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

[akhuang]

Summary

This patch modifies the data layout for the X86 target. This is for an upcoming change that adds three new 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.

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–779

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–779

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.