This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Set a data layout entry for buffer descriptors (addrspace 7)
AbandonedPublic

Authored by krzysz00 on Feb 7 2023, 12:45 PM.

Details

Summary

Buffer descriptors, which we will eventually want to treat as pointers
in their own address space, are 128-bit-long values that are indexed
by a 32-bit offset. These values consist of 48 bits of pointer and 96
bits of metadata.

This commit amends the data layout string for the GCN+ subtarget to
preperly describe buffer descriptors instead of having them default to
a 64-bit size.

Future work:

  • The "offset is 32 bits" part of the data layout description doesn't

make it through IRTranslator, which currently assumes that the pointer
size is the same as the indexing size. This will be resolved in a
separate commit for atomicity.

  • emitIntValue() in AsmPrinter cannot handle values of size over 64

bits and crashes as a result. This makes nulls.ll crash - the relevant
line has been commented out pending a fix. ( D143515 was a hack to
make this test pass, initial review feedback suggested landing this
change first.)

This commit also updates all tests that explicitly contain the GCN
layout string to contain its new value.

Depends on D143437

Diff Detail

Event Timeline

krzysz00 created this revision.Feb 7 2023, 12:45 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 7 2023, 12:45 PM
krzysz00 requested review of this revision.Feb 7 2023, 12:45 PM
arsenm added a comment.Feb 7 2023, 1:29 PM

Should bitcode auto upgrade try to fix the datalayout?

llvm/test/CodeGen/AMDGPU/nullptr.ll
35

Better to move this to a separate xfail test

krzysz00 updated this revision to Diff 495861.Feb 8 2023, 8:25 AM

Add autoupgrade, reorganize tests.

krzysz00 updated this revision to Diff 496515.Feb 10 2023, 9:17 AM

Apparently clang has its own copy of this data layout.

Herald added a project: Restricted Project. · View Herald TranscriptFeb 10 2023, 9:17 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Discussion related to this is also happening over at https://reviews.llvm.org/D143945 .

arsenm accepted this revision.Feb 14 2023, 10:52 AM
This revision is now accepted and ready to land.Feb 14 2023, 10:52 AM

This really needs to be a 160-bit type, not a 128-bit type.

krzysz00 abandoned this revision.Mar 7 2023, 8:07 AM