Page MenuHomePhabricator

Add a default address space for globals to DataLayout
Needs ReviewPublic

Authored by arichardson on Dec 3 2019, 2:07 AM.

Details

Summary

This is similar to the existing alloca and program address spaces (D37052)
and should be used when creating/accessing global variables.
We need this in our CHERI fork of LLVM to place all globals in address space 200.
This ensures that values are accessed using CHERI load/store instructions
instead of the normal MIPS/RISC-V ones.

The problem this is trying to fix is that most of the time the type of
globals is created using a simple PointerType::getUnqual() (or ::get() with
the default address-space value of 0). This does not work for us and we get
assertion/compilation/instruction selection failures whenever a new call
is added that uses the default value of zero.

In our fork we have removed the default parameter value of zero for most
address space arguments and use DL.getProgramAddressSpace() or
DL.getGlobalsAddressSpace() whenever possible. If this change is accepted,
I will upstream follow-up patches to use DL.getGlobalsAddressSpace() instead
of relying on the default value of 0 for PointerType::get(), etc.

This patch and the follow-up changes will not have any functional changes
for existing backends with the default globals address space of zero.

Diff Detail

Unit TestsFailed

TimeTest
2,610 msLLVM.CodeGen/SPARC::fp128.ll
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/llc < /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/llvm/test/CodeGen/SPARC/fp128.ll -march=sparc -mattr=hard-quad-float | /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/FileCheck /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/llvm/test/CodeGen/SPARC/fp128.ll --check-prefix=CHECK --check-prefix=HARD --check-prefix=BE

Event Timeline

arichardson created this revision.Dec 3 2019, 2:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 3 2019, 2:07 AM

Build result: fail - 60406 tests passed, 1 failed and 726 were skipped.

failed: LLVM.CodeGen/SPARC/fp128.ll

Log files: console-log.txt, CMakeCache.txt

arsenm added a comment.Dec 3 2019, 8:55 AM

Needs some bitcode compatibility tests, and actual uses. AMDGPU's data layout should also add -G1

llvm/docs/LangRef.rst
2059

The phrasing is a bit wrong. This implies for all global variables. This should maybe be the default address space for globals?

arichardson marked an inline comment as done.Dec 3 2019, 9:25 AM

Needs some bitcode compatibility tests, and actual uses. AMDGPU's data layout should also add -G1

That's great to know and should make it easier to write tests.

I was going to add uses in follow-up commits but if you prefer I can also include some in this change.

llvm/docs/LangRef.rst
2059

Possibly something like this? Specifies the address space to be used by default when creating global variables. If omitted, the globals address space defaults to the default address space 0. Note: variable declarations without an address space are always created in address space 0, this property only affects the default value to be used when creating globals programmatically.

I could probably also adjust the parser to use this value as a default but I'm not sure that is desirable.

Recommend adding a couple new tests to unittests/IR/DataLayoutTest.cpp.