Page MenuHomePhabricator

Add support for multiple program address spaces
AbandonedPublic

Authored by pmatos on Nov 13 2020, 7:45 AM.

Details

Summary

Allows for multiple program address spaces to be defined through
multiple Px-Py... in the data layout.
If none is specified, the default program address space is 0.

Unless data layout is modified with multiple Px, this patch shouldn't
have any effect on current code.

Diff Detail

Unit TestsFailed

TimeTest
380 mslinux > HWAddressSanitizer-x86_64.TestCases::sizes.cpp
Script: -- : 'RUN: at line 3'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang --driver-mode=g++ -m64 -gline-tables-only -fsanitize=hwaddress -fuse-ld=lld -mcmodel=large -mllvm -hwasan-globals -mllvm -hwasan-use-short-granules -mllvm -hwasan-instrument-landing-pads=0 -mllvm -hwasan-instrument-personality-functions /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/hwasan/TestCases/sizes.cpp -nostdlib++ -lstdc++ -o /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/hwasan/X86_64/TestCases/Output/sizes.cpp.tmp
290 mslinux > HWAddressSanitizer-x86_64.TestCases/Linux::reuse-threads.cpp
Script: -- : 'RUN: at line 2'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang --driver-mode=g++ -m64 -gline-tables-only -fsanitize=hwaddress -fuse-ld=lld -mcmodel=large -mllvm -hwasan-globals -mllvm -hwasan-use-short-granules -mllvm -hwasan-instrument-landing-pads=0 -mllvm -hwasan-instrument-personality-functions -mllvm -hwasan-instrument-stack=0 /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/hwasan/TestCases/Linux/reuse-threads.cpp -o /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/hwasan/X86_64/TestCases/Linux/Output/reuse-threads.cpp.tmp && env HWASAN_OPTIONS=disable_allocator_tagging=1:random_tags=0:verbose_threads=1 /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/hwasan/X86_64/TestCases/Linux/Output/reuse-threads.cpp.tmp 2>&1 | FileCheck /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/hwasan/TestCases/Linux/reuse-threads.cpp

Event Timeline

pmatos created this revision.Nov 13 2020, 7:45 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptNov 13 2020, 7:45 AM
pmatos requested review of this revision.Nov 13 2020, 7:45 AM

This is WIP - there a still a few test failures but I am happy to start getting comments on this.

This is in preparation for implementation of reference types for the WebAssembly backend that requires functions to be able to live in multiple address spaces.

I think this is a good direction overall, and I'm glad the code doesn't become any messier with this change. I do think it would be good to also email llvm-dev about this change to get general feedback and make sure it doesn't require a full RFC.

llvm/lib/AsmParser/LLParser.cpp
1468–1469

Or maybe this comment can be removed since it is subsumed by the comment you added just below.

1490

It would be good to add a test for this error message (if one doesn't already exist). It's not obvious to me that the expected type in the error message will always have a correct program address space.

llvm/lib/IR/DataLayout.cpp
476

It would be good to check for repeats here. As it is, hasNonZeroProgramAddressSpace() would return true for "P0-P0" .

pmatos updated this revision to Diff 305488.Nov 16 2020, 5:52 AM

Ensure the program address spaces vector doesn't contain duplicates.

pmatos updated this revision to Diff 305521.Nov 16 2020, 8:07 AM

Fix type check.

jdoerfert requested changes to this revision.Nov 16 2020, 8:26 AM
jdoerfert added a subscriber: tianshilei1992.

I'll be on the lookout for the RFC. There, and in an updated commit message, you have to provide more details.

http://lists.llvm.org/pipermail/llvm-dev/2020-July/143808.html is related as well.

This revision now requires changes to proceed.Nov 16 2020, 8:26 AM
arsenm added a subscriber: arsenm.Nov 17 2020, 12:00 PM

While the IR definitely should support mixing functions with different address spaces, I don't see the point of encoding multiple of these in the datalayout

llvm/include/llvm/IR/DataLayout.h
125–129

I don't see why we need a list of program address spaces. The datalayout only needs to know the one to use for situations when generic code needs to synthesize a function out of thin air. Anything beyond that would need target information in the specific pass.

@arsenm, Are you suggesting that we just relax the verification rules to allow calling function pointers to arbitrary address spaces without needing any changes to the data layout string? That seems fine to me, but the data layout string solution does allow targets to opt in to more precise validation.

arichardson requested changes to this revision.Nov 23 2020, 12:08 PM

I don't see why you would need multiple program address spaces to support calls to other address spaces. You can already do the following:

define i32 @foo(i32) addrspace(1) {
    %ret = add i32 %0, 1
    ret i32 %ret
}

define i32 @bar() addrspace(0) {
    %call = call addrspace(1) i32 @foo(i32 1)
    ret i32 %call
}

Isn't that sufficient for your WebAssembly changes?

arichardson added a comment.EditedNov 23 2020, 12:12 PM

The change to rename getProgramAddressSpace getDefaultProgramAddressSpace seems fine to me since it matches the GlobalsAddressSpace.

By the way, your test already seems to work (if you add an explicit call addrspace(1) void %ref() for llc: https://godbolt.org/z/arj9M9

jrtc27 added a subscriber: jrtc27.EditedNov 23 2020, 12:21 PM

Currently P0-P1 is valid and results in the program address space being 1, but this patch changes the semantics of that. How sure are you that nothing will break? I do not like the idea of reusing existing valid syntax to mean something else; if you want to introduce some notion of secondary program address spaces then the syntax should be different.

However, I don't understand why this is needed; can you not just change your call void %ref() to call addrspace(1) void %ref()? That gets parsed and type-checked just fine for me, i.e. this is not adding anything significant to LLVM other than letting you be lazy and drop the address space for calls through function pointers to a non-default address space?

Thanks, @arichardson and @jrtc27 for your comments.
I am definitely surprised to find that if you explicitly mark the call with the address space, this patch is not required. At first look, this RFC is not required any more but I need sometime to investigate further. If no changes are necessary, this is indeed good news.

pmatos abandoned this revision.Nov 30 2020, 10:51 AM

Thanks, @arichardson and @jrtc27 for your comments.
I am definitely surprised to find that if you explicitly mark the call with the address space, this patch is not required. At first look, this RFC is not required any more but I need sometime to investigate further. If no changes are necessary, this is indeed good news.

Thank you for your comments - I am dropping this patch and the RFC as it is indeed not required by my use case.