Support for the program data address space option of LLVM's Target Datalayout #54993

TimNN · 2018-10-11T15:52:37Z

This was introduced recently (specifically, for AVR, cc @dylanmckay).

(I came up with this when attempting to run avr-rust rebased on the latest rust-lang commits. If this requires a different design, some additional discussions, or is not something to pursue right now, I'd be happy to close this PR).

Note that this somewhat overlaps with @DiamondLovesYou's #51576, I think, although the implementation here is significantly simpler: Since the address space applies to all program data, we can just check the pointee's type whenever we create an LLVM pointer type. If it is a function we use the program data address space; if not we use the default address space.

cc @eddyb, who has been reviewing #51576

Ref: https://llvm.org/docs/LangRef.html#data-layout

rust-highfive · 2018-10-11T15:52:48Z

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

rust-highfive · 2018-10-11T16:12:20Z

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.


[00:04:44] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:04:44] tidy error: /checkout/src/librustc_target/abi/mod.rs:109: line longer than 100 chars
[00:04:45] some tidy checks failed
[00:04:45] 
[00:04:45] 
[00:04:45] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:04:45] 
[00:04:45] 
[00:04:45] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:04:45] Build completed unsuccessfully in 0:00:47
[00:04:45] Build completed unsuccessfully in 0:00:47
[00:04:45] make: *** [tidy] Error 1
[00:04:45] Makefile:79: recipe for target 'tidy' failed

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:0cdda034
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
---
travis_time:end:02bd36e4:start=1539273876699334755,finish=1539273876704878228,duration=5543473
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:01dbbf09
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:0531b499
travis_time:start:0531b499
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:19dc590e
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

nikomatsakis · 2018-10-16T15:19:42Z

src/librustc_codegen_llvm/type_.rs

+        if self.kind() == TypeKind::Function {
+            unsafe {
+                llvm::LLVMPointerType(self,
+                                      cx.data_layout().program_memory_address_space as c_uint)


It is possible to "mis-use" this, I assume, for example via transmutes or something? Can we wind up with "valid Rust code" giving weird LLVM assertion failures or other errors?

That's a good point. I'll see what happens if I transmute a few things to a function pointer.

nikomatsakis · 2018-10-16T15:20:12Z

cc @rust-lang/compiler — seems like an ad-hoc extension, nominating for brief T-compiler discussion to decide best path

TimNN · 2018-10-16T15:32:06Z

seems like an ad-hoc extension

Yeah, this is definitely a bit ad-hoc, however overall the approach doesn't seem too bad to me. In any case, I came up with this while trying to get a target to work that isn't even officially supported yet. I'd be totally happy if you said this requires some more design work in the future.

nikomatsakis · 2018-10-17T19:18:53Z

@TimNN to be clear, I didn't mean "ad hoc" in the pejorative sense. Just that it's one of those "small features" that we often decide without an RFC -- it might however warrant a feature gate or a tracking issue or some such thing.

DiamondLovesYou · 2018-10-18T06:32:08Z

My patch can be easily extended to support this. But I think the space should be called the instruction address space, since "program memory" is kinda ambiguous (even though this is the term LLVM uses).

@eddyb wanted any value to be cast into the flat address space before that value is used (ie uses of a global would actually use an addrspacecast-ed const expr), in order to reduce the patch's complexity footprint. @TimNN would this be possible for the AVR target machine? The initial version I've whipped up doesn't force this backflip (it uses the correct space for functions when creating LLVM types from our types), but @eddyb might want them to go through a flat space cast round trip.

TimNN · 2018-10-18T07:07:21Z

think the space should be called the instruction address space

I'd be fine with that, as you noticed I was just copying LLVM terminology.

@eddyb wanted any value to be cast into the flat address space before that value is used (ie uses of a global would actually use an addrspacecast-ed const expr), in order to reduce the patch's complexity footprint. @TimNN would this be possible for the AVR target machine?

I don't know. I understand how my patch works but overall I'm mostly unfamiliar with the involved concepts. IIRC, however, LLVM was unhappy casting a function to a function pointer in an incorrect addrspace (In case that information helps you).

In any case, if you think that the functionality added here could be better supported with your patch, I'd be happy to wait for that to be merged. (I'd probably open this PR against the avr-rust repo for others to use in the meantime, but for me it is in no way urgent to get this submitted).

bors · 2018-10-18T09:56:18Z

☔ The latest upstream changes (presumably #55171) made this pull request unmergeable. Please resolve the merge conflicts.

eddyb · 2018-10-18T15:13:26Z

src/librustc_codegen_llvm/type_.rs

@@ -233,7 +234,26 @@ impl Type {
        }
    }

-    pub fn ptr_to(&self) -> &Type {
+    pub fn ptr_to(&self, cx: &CodegenCx<'ll, '_>) -> &Type {
+        if self.kind() == TypeKind::Function {


I really want to avoid us introspecting LLVM types. I think we should either not use non-flat address space pointers or we should special-case fn pointers - from a quick search, I found only 2 places that create them:

rust/src/librustc_codegen_llvm/type_of.rs

Line 268 in 40123a1

FnType::new(cx, sig, &[]).llvm_type(cx).ptr_to()

rust/src/librustc_codegen_llvm/meth.rs

Line 42 in 40123a1

let llvtable = bx.pointercast(llvtable, fn_ty.llvm_type(bx.cx).ptr_to().ptr_to());

Since these are both calling .ptr_to() on the result of FnType::llvm_type, a helper method on FnType can probably be added.

eddyb · 2018-10-18T15:18:19Z

src/librustc_target/abi/mod.rs

@@ -96,6 +106,8 @@ impl TargetDataLayout {
            match spec.split(':').collect::<Vec<_>>()[..] {
                ["e"] => dl.endian = Endian::Little,
                ["E"] => dl.endian = Endian::Big,
+                [ref p] if p.starts_with("P") =>


Suggested change

[ref p] if p.starts_with("P") =>

[p] if p.starts_with("P") =>

DiamondLovesYou · 2018-10-20T10:00:01Z

@TimNN Well, I've push a change to support the feature needed in this PR, so feel free to give it a try.

I don't know. I understand how my patch works but overall I'm mostly unfamiliar with the involved concepts. IIRC, however, LLVM was unhappy casting a function to a function pointer in an incorrect addrspace (In case that information helps you).

It does help. As I said, my patch put the vtable entries and all declared functions in the instruction address space, so that should work. I just realized it would not, however, do the same for function ptrs cast from non fn ptr values, like integers (perhaps a bit of an edge case). Currently, it tries to cast the value to the flat address space just like any other ptr.

But, baring that, it should work for AVR with a suitable target spec. If you'd like to give my patch a shot, let me know how it goes!

I would definitely prefer to have my patch merged instead; it (now) supports (that int -> fn ptr issue not withstanding) this address space, and can compile Rust which then runs on an AMDGPU.

TimNN · 2018-10-22T14:50:31Z

Thanks for your reviews, @eddyb & @nikomatsakis!

I'm not sure what you think, but I feel like @DiamondLovesYou's PR #51576 is the way to go in the long term.

I however don't know what the timeline would be to get that merged (or if you maybe have concerns that would prevent merging it).

I see two ways forward:

close this for now until a decision on Make librustc_codegen_llvm aware of LLVM address spaces. #51576 is reached.
merge this for now (after addressing your comments) and refactor / upgrade things in Make librustc_codegen_llvm aware of LLVM address spaces. #51576.

What do you all think?

shepmaster · 2018-10-22T16:30:53Z

A drive-by comment from AVR-land...

We think that users would like / need the ability to annotate certain pieces of memory with which address space they should go in. We opened an issue about this, but in short, something like this strawman syntax could address the issue:

#[space(progmem)]
pub static FOOBAR: &'static str = "hello world";

I don't know that this PR would handle something like that, but it may be worth keeping in mind for the underlying design.

cc rust-lang#54993

TimNN · 2018-10-30T17:41:18Z

I've updated this PR to address the review comments. However I'll close this for now until there's a decision on ##51576.

rust-highfive · 2018-10-30T17:41:55Z

Your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@dylanmckay

Support for the program data address space option of LLVM's Target Datalayout This was introduced recently (specifically, for AVR, cc @dylanmckay). (I came up with this when attempting to run [avr-rust](/~https://github.com/avr-rust/rust) rebased on the latest [rust-lang](/~https://github.com/rust-lang/rust) commits. If this requires a different design, some additional discussions, or is not something to pursue right now, I'd be happy to close this PR). Note that this somewhat overlaps with @DiamondLovesYou's rust-lang#51576, I think, although the implementation here is significantly simpler: Since the address space applies to _all_ program data, we can just check the pointee's type whenever we create an LLVM pointer type. If it is a function we use the program data address space; if not we use the default address space. cc @eddyb, who has been reviewing rust-lang#51576 Ref: https://llvm.org/docs/LangRef.html#data-layout

bors · 2018-11-09T00:04:53Z

⌛ Testing commit 50a2d47 with merge f705d13c0de0f6721468e867ae993bbf819ddbbd...

bors · 2018-11-09T03:08:22Z

💔 Test failed - status-appveyor

kennytm · 2018-11-09T03:14:06Z

@bors retry

3 hour timeout in x86_64-msvc, likely due to LLVM cache purge.

bors · 2018-11-09T14:24:19Z

⌛ Testing commit 50a2d47 with merge b3a9ad420ecc2b738cc66a2437a865ec6f6f648d...

bors · 2018-11-09T17:26:50Z

💔 Test failed - status-appveyor

TimNN · 2018-11-10T21:47:05Z

@bors retry

Another timeout on AppVeyor due to LLVM rebuilding?

bors · 2018-11-11T03:44:25Z

⌛ Testing commit 50a2d47 with merge 9b8f902...

@dylanmckay

Support for the program data address space option of LLVM's Target Datalayout This was introduced recently (specifically, for AVR, cc @dylanmckay). (I came up with this when attempting to run [avr-rust](/~https://github.com/avr-rust/rust) rebased on the latest [rust-lang](/~https://github.com/rust-lang/rust) commits. If this requires a different design, some additional discussions, or is not something to pursue right now, I'd be happy to close this PR). Note that this somewhat overlaps with @DiamondLovesYou's #51576, I think, although the implementation here is significantly simpler: Since the address space applies to _all_ program data, we can just check the pointee's type whenever we create an LLVM pointer type. If it is a function we use the program data address space; if not we use the default address space. cc @eddyb, who has been reviewing #51576 Ref: https://llvm.org/docs/LangRef.html#data-layout

bors · 2018-11-11T06:26:14Z

☀️ Test successful - status-appveyor, status-travis
Approved by: eddyb
Pushing 9b8f902 to master...

rust-highfive assigned nikomatsakis Oct 11, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 11, 2018

TimNN force-pushed the pda-tdl branch from 2d7e965 to abceafe Compare October 15, 2018 15:49

nikomatsakis reviewed Oct 16, 2018

View reviewed changes

eddyb reviewed Oct 18, 2018

View reviewed changes

TimNN mentioned this pull request Oct 22, 2018

[LLVM 8.0] Assertion failed: Trying to add an operand to a machine instr that is already done! avr-rust/rust-legacy-fork#112

Closed

TimNN added a commit to TimNN/rust that referenced this pull request Oct 28, 2018

[AVR] Rustc: support instruction address space

34e01a2

cc rust-lang#54993

TimNN force-pushed the pda-tdl branch 3 times, most recently from 5a2494b to 465ba9d Compare October 30, 2018 17:40

TimNN closed this Oct 30, 2018

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 8, 2018

kennytm mentioned this pull request Nov 8, 2018

Rollup of 16 pull requests #55789

Closed

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 9, 2018

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 9, 2018

bors mentioned this pull request Nov 9, 2018

Do not attempt to ascribe projections out of a ty var #55637

Merged

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 9, 2018

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 10, 2018

bors merged commit 50a2d47 into rust-lang:master Nov 11, 2018

TimNN deleted the pda-tdl branch November 11, 2018 09:34

peterhj mentioned this pull request Dec 3, 2018

Global and device kernels are unsound rust-cuda/wg#11

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for the program data address space option of LLVM's Target Datalayout #54993

Support for the program data address space option of LLVM's Target Datalayout #54993

TimNN commented Oct 11, 2018 •

edited

Loading

rust-highfive commented Oct 11, 2018

rust-highfive commented Oct 11, 2018

nikomatsakis Oct 16, 2018

TimNN Oct 16, 2018

nikomatsakis commented Oct 16, 2018

TimNN commented Oct 16, 2018

nikomatsakis commented Oct 17, 2018

DiamondLovesYou commented Oct 18, 2018

TimNN commented Oct 18, 2018

bors commented Oct 18, 2018

eddyb Oct 18, 2018

nikomatsakis Oct 18, 2018

eddyb Oct 18, 2018

DiamondLovesYou commented Oct 20, 2018

TimNN commented Oct 22, 2018

shepmaster commented Oct 22, 2018

TimNN commented Oct 30, 2018

rust-highfive commented Oct 30, 2018

bors commented Nov 9, 2018

bors commented Nov 9, 2018

kennytm commented Nov 9, 2018

bors commented Nov 9, 2018

bors commented Nov 9, 2018

TimNN commented Nov 10, 2018

bors commented Nov 11, 2018

bors commented Nov 11, 2018

	[ref p] if p.starts_with("P") =>
	[p] if p.starts_with("P") =>

Support for the program data address space option of LLVM's Target Datalayout #54993

Support for the program data address space option of LLVM's Target Datalayout #54993

Conversation

TimNN commented Oct 11, 2018 • edited Loading

rust-highfive commented Oct 11, 2018

rust-highfive commented Oct 11, 2018

nikomatsakis Oct 16, 2018

Choose a reason for hiding this comment

TimNN Oct 16, 2018

Choose a reason for hiding this comment

nikomatsakis commented Oct 16, 2018

TimNN commented Oct 16, 2018

nikomatsakis commented Oct 17, 2018

DiamondLovesYou commented Oct 18, 2018

TimNN commented Oct 18, 2018

bors commented Oct 18, 2018

eddyb Oct 18, 2018

Choose a reason for hiding this comment

nikomatsakis Oct 18, 2018

Choose a reason for hiding this comment

eddyb Oct 18, 2018

Choose a reason for hiding this comment

DiamondLovesYou commented Oct 20, 2018

TimNN commented Oct 22, 2018

shepmaster commented Oct 22, 2018

TimNN commented Oct 30, 2018

rust-highfive commented Oct 30, 2018

bors commented Nov 9, 2018

bors commented Nov 9, 2018

kennytm commented Nov 9, 2018

bors commented Nov 9, 2018

bors commented Nov 9, 2018

TimNN commented Nov 10, 2018

bors commented Nov 11, 2018

bors commented Nov 11, 2018

TimNN commented Oct 11, 2018 •

edited

Loading