Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make librustc_codegen_llvm aware of LLVM address spaces. #51576

Closed

Conversation

DiamondLovesYou
Copy link
Contributor

@DiamondLovesYou DiamondLovesYou commented Jun 15, 2018

In order to not require overloading functions based on their argument's address
space (among other things), we require the presence of a "flat" (ie an address
space which is shared with every other address space) address space.

This isn't exposed in any way to Rust code. This just makes Rust compatible with
LLVM target machines which, for example, place allocas in a different address
space. amdgcn-amd-amdhsa-amdgiz is a specific example, which places allocas in
address space 5 or the private (at the work item level) address space. This
includes working with nonuniform sized pointers.

@rust-highfive
Copy link
Collaborator

r? @petrochenkov

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 15, 2018
Copy link
Member

@Mark-Simulacrum Mark-Simulacrum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rustbuild changes look acceptable to me.

@@ -38,6 +38,7 @@ use cache::Interned;
pub struct Llvm {
pub target: Interned<String>,
pub emscripten: bool,
pub dump_enabled: bool,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be no need for this here as AFAICT it's always globally set, we never do anything but pass it in from config.llvm_dump_enabled.

@@ -15,7 +15,7 @@ use llvm::{AtomicRmwBinOp, AtomicOrdering, SynchronizationScope, AsmDialect};
use llvm::{Opcode, IntPredicate, RealPredicate, False, OperandBundleDef};
use llvm::{ValueRef, BasicBlockRef, BuilderRef, ModuleRef};
use common::*;
use type_::Type;
use type_::{Type, };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed!

@@ -10,6 +10,7 @@ crate-type = ["dylib"]

[dependencies]
bitflags = "1.0"
syntax_pos = { path = "../libsyntax_pos" }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see this dependency actually being used -- but maybe I'm missing something?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rustc_target shouldn't have any dependencies on the compiler, so hopefully this is unused.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is used for custom address space names.

Oh, it was. I forgot I had removed it.

@Mark-Simulacrum
Copy link
Member

r? @eddyb probably

@rust-highfive rust-highfive assigned eddyb and unassigned petrochenkov Jun 15, 2018
@@ -553,6 +553,13 @@ impl Handler {
db.emit();
panic!(ExplicitBug);
}
pub fn bug_no_panic(&self, msg: &str) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eddyb
Copy link
Member

eddyb commented Jun 15, 2018

cc @alexcrichton

I'm still reading, but some of the details seem a bit more invasive than I expected. reviewed

@@ -47,3 +47,4 @@ jemalloc = ["rustc_target/jemalloc"]
# when this option is enabled or not. That way we can build two, cache two
# artifacts, and have nice speedy rebuilds.
emscripten = ["rustc_llvm/emscripten"]
dump = ["rustc_llvm/dump"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing newline at the end of file.

bx.pointercast(dst.llval, Type::i8p(cx)),
bx.pointercast(llscratch, Type::i8p(cx)),
bx.pointercast(dst.llval, ti8.ptr_to_as(dst_addr_space)),
bx.pointercast(llscratch, ti8.ptr_to_as(addr_space)),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is common, wouldn't a replacement for pointercast that only casts the pointee, but preserves the AS, be better?

@@ -567,7 +571,9 @@ impl<'a, 'tcx> FnTypeExt<'a, 'tcx> for FnType<'tcx, Ty<'tcx>> {
}
PassMode::Cast(cast) => cast.llvm_type(cx),
PassMode::Indirect(_) => {
llargument_tys.push(self.ret.memory_ty(cx).ptr_to());
let ty = self.ret.memory_ty(cx)
.ptr_to_as(cx.alloca_addr_space());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of having a method to retrieve a specific AS and then another to create a pointer type, why not have e.g. .alloca_ptr_to(cx) or .ptr_to(cx, AddrSpaceKind::Alloca) (okay, the latter is a bit long).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually already have those functions, I just probably added them after I wrote some of this 😄

llargument_tys.push(arg.layout.scalar_pair_element_llvm_type(cx, 1));
let data_ty = arg.layout.scalar_pair_element_llvm_type(cx, 0)
.as_flat_addr_space(cx);
// Keep the original addrspace, dst's of course aren't ptrs,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By "dst" you mean "slice (length)"? Also, I should note this doesn't necessarily work.
The pair could just as well be (&T, &U) where T: Sized, U: Sized.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A better approach might be to pass something extra to scalar_pair_element_llvm_type. Same for everything else, really.

(bx.pointercast(src, ptr_ty), unsized_info(bx.cx, a, b, None))
}
(&ty::TyAdt(def_a, _), &ty::TyAdt(def_b, _)) if def_a.is_box() && def_b.is_box() => {
let (a, b) = (src_ty.boxed_ty(), dst_ty.boxed_ty());
assert!(bx.cx.type_is_sized(a));
let ptr_ty = bx.cx.layout_of(b).llvm_type(bx.cx).ptr_to();
let ptr_ty = bx.cx.layout_of(b).llvm_type(bx.cx)
.ptr_to_as(val_ty(src).address_space());
(bx.pointercast(src, ptr_ty), unsized_info(bx.cx, a, b, None))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, regarding the usecase of only needed to cast the pointee type.

}
}
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let g = declare::define_global(cx, &sym[..], val_ty(sc)).unwrap_or_else(||{
bug!("symbol `{}` is already defined", sym);
});
let addr_space = cx.default_const_addr_space();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe pass the AddrSpaceKind enum value around instead of the LLVM AS number?

@@ -64,13 +64,16 @@ fn set_global_alignment(cx: &CodegenCx,
pub fn addr_of_mut(cx: &CodegenCx,
cv: ValueRef,
align: Align,
kind: &str)
kind: &str,
addr_space: Option<AddrSpaceIdx>)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There shouldn't be that many calls, making this non-optional (and using AddrSpaceKind) would be better IMO.

pub alloca_i8p_ty: Type,
pub const_i8p_ty: Type,
pub global_i8p_ty: Type,
pub flat_i8p_ty: Type,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably shouldn't have cached types like these.

pub dbg_cx: Option<debuginfo::CrateDebugContext<'tcx>>,

eh_personality: Cell<Option<ValueRef>>,
eh_unwind_resume: Cell<Option<ValueRef>>,
pub rust_try_fn: Cell<Option<ValueRef>>,

intrinsics: RefCell<FxHashMap<&'static str, ValueRef>>,
intrinsics: RefCell<FxHashMap<Cow<'static, str>, ValueRef>>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is worse, because it incurs extra branching. Might as well switch it to String.

ifn!("llvm.memset.p0i8.i32", fn(i8p, t_i8, t_i32, t_i32, i1) -> void);
ifn!("llvm.memset.p0i8.i64", fn(i8p, t_i8, t_i64, t_i32, i1) -> void);
if key.starts_with("llvm.memcpy.") || key.starts_with("llvm.memmove") ||
key.starts_with("llvm.memset") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

memcpy has a dot at the end, but the others don't?

llpair = bx.insert_value(llpair, a, 0);
let b = b.copy_addr_space(bx, val_ty(llpair).struct_element_type(1));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would these two be needed?

PlaceRef {
// HACK(eddyb) have to bitcast pointers until LLVM removes pointee types.
llval: bx.pointercast(llval, field.llvm_type(cx).ptr_to()),
llval: bx.pointercast(llval, dest_ty),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, could have a pointercast variant that only casts the pointee.

let asp = val_ty(downcast.llval).address_space();
downcast.llval = bx
.pointercast(downcast.llval,
variant_ty.ptr_to_as(asp));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

@@ -17,6 +17,7 @@ use rustc::middle::lang_items::ExchangeMallocFnLangItem;
use rustc_apfloat::{ieee, Float, Status, Round};
use std::{u128, i128};

use abi::{FnType, FnTypeExt, };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Superfluous , .

} else {
fn_ret_val
};
let val = bx.pointercast(ret_val, llty_ptr);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this all doing? Why would the function being called here ever return by indirection?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Accidentally included. Was created when I was trying to figure out how to work with the AMDGPU target machine. I tried having every function be the amdgpu kernel abi, which doesn't allow non-void return types.

Removed in the next version, to be pushed Soon(TM).

} else {
cx.default_global_addr_space()
}
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should avoid touching the HIR here, but it's not clear what requirements there actually are.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need the address space of the global for the global's LLVM type, meaning it needs to compute the same address space as consts::codegen_static.

At the time, I wasn't aware of a better way, but the next version now does what MonoItemExt::define does.

pub fn is_array(&self) -> bool { self.kind() == llvm::TypeKind::Array }
pub fn is_vector(&self) -> bool { self.kind() == llvm::TypeKind::Vector }
pub fn is_struct(&self) -> bool { self.kind() == llvm::TypeKind::Struct }
pub fn is_func(&self) -> bool { self.kind() == llvm::TypeKind::Function }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why all of these? We should avoid introspecting LLVM types at all costs IMO.

Copy link
Member

@eddyb eddyb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see how any of the code around read-only and read-write global address spaces, actually works. Why are vtable pointers different from any other pointer?
If there is a flat address space, why isn't it used more often? Some documentation would help.

I hope there is a simpler way to support the target platform you want.

@pietroalbini pietroalbini added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jun 25, 2018
@mati865
Copy link
Contributor

mati865 commented Dec 10, 2018

This PR was labelled as Blocked 2 months ago:

Waiting on people to be freed up from work on the 2018 edition

@BatmanAoD is it still the case?

@pietroalbini pietroalbini added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels Dec 11, 2018
@pietroalbini
Copy link
Member

Review ping @eddyb

@bors
Copy link
Contributor

bors commented Dec 13, 2018

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

.unwrap_or_default();
let instruction_addr_space =
tcx.sess.target.target.options.addr_spaces
.get(&AddrSpaceKind::Flat)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be Instruction

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, thanks

let kinds = vec![AddrSpaceKind::ReadOnly,
AddrSpaceKind::ReadWrite,
AddrSpaceKind::Alloca,
AddrSpaceKind::Flat, ];
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing Instruction?

@DiamondLovesYou DiamondLovesYou force-pushed the rustc-trans-addr-space branch 2 times, most recently from b719335 to 7a13695 Compare December 25, 2018 17:03
@bors
Copy link
Contributor

bors commented Dec 26, 2018

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

@DiamondLovesYou DiamondLovesYou force-pushed the rustc-trans-addr-space branch 2 times, most recently from 286b70a to f0abb1b Compare December 30, 2018 23:14
@bors
Copy link
Contributor

bors commented Jan 5, 2019

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

@bors
Copy link
Contributor

bors commented Jan 16, 2019

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

In order to not require overloading functions based on their argument's address
space (among other things), we require the presence of a "flat" (ie an address
space which is shared with every other address space) address space.

This isn't exposed in any way to Rust code. This just makes Rust compatible with
LLVM target machines which, for example, place allocas in a different address
space. `amdgcn-amd-amdhsa-amdgiz` is a specific example, which places allocas in
address space 5 or the private (at the work item level) address space.
@bors
Copy link
Contributor

bors commented Feb 9, 2019

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

@Dylan-DPC-zz
Copy link

ping from triage @DiamondLovesYou you have conflicts you need to resolve

@Dylan-DPC-zz Dylan-DPC-zz added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 4, 2019
@Dylan-DPC-zz
Copy link

ping from triage @DiamondLovesYou
Unfortunately we haven't heard from you on this in a while, so I'm closing the PR to keep things tidy. Don't worry though, if you'll have time again in the future please reopen this PR, we'll be happy to review it again! Thanks for contirbuting!

@Dylan-DPC-zz Dylan-DPC-zz added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.