This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by akhuang on Jul 18 2019, 9:44 AM.

Details

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.

Event Timeline

akhuang created this revision.Jul 18 2019, 9:44 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJul 18 2019, 9:44 AM

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

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.

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 added inline comments.Aug 8 2019, 11:07 AM
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 added inline comments.Aug 8 2019, 11:35 AM
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.

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 updated this revision to Diff 214202.Aug 8 2019, 11:53 AM

Remove test case changes.

lebedev.ri added inline comments.
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.

@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 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?

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.

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?

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

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.

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

arsenm added a subscriber: arsenm.Aug 9 2019, 8:28 AM

@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?

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 updated this revision to Diff 215452.Aug 15 2019, 11:54 AM
  • 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 edited the summary of this revision. (Show Details)Aug 15 2019, 11:54 AM

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?

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 updated this revision to Diff 216894.Aug 23 2019, 10:26 AM
  • Change datalayout in lld test cases.
akhuang edited the summary of this revision. (Show Details)Aug 26 2019, 4:00 PM
rnk accepted this revision.Aug 26 2019, 4:07 PM

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

This revision is now accepted and ready to land.Aug 26 2019, 4:07 PM

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

akhuang closed this revision.Aug 27 2019, 10:59 AM

Commited in r370083

dyung added a subscriber: dyung.Aug 27 2019, 2:57 PM

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 added a comment.Aug 27 2019, 3:07 PM

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?