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

Add rustc_intrinsic_const_vector_arg attribute to allow vectors to be passed as constants #118980

Closed
wants to merge 15 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock
Original file line number Diff line number Diff line change
Expand Up @@ -3779,6 +3779,7 @@ dependencies = [
"either",
"itertools",
"polonius-engine",
"rustc_ast",
"rustc_data_structures",
"rustc_errors",
"rustc_fluent_macro",
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_borrowck/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ edition = "2021"
either = "1.5.0"
itertools = "0.12"
polonius-engine = "0.13.0"
rustc_ast = { path = "../rustc_ast" }
rustc_data_structures = { path = "../rustc_data_structures" }
rustc_errors = { path = "../rustc_errors" }
rustc_fluent_macro = { path = "../rustc_fluent_macro" }
Expand Down
2 changes: 2 additions & 0 deletions compiler/rustc_borrowck/messages.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,8 @@ borrowck_higher_ranked_lifetime_error =
borrowck_higher_ranked_subtype_error =
higher-ranked subtype error

borrowck_intrinsic_const_vector_arg_non_const = argument at index {$index} must be a constant

borrowck_lifetime_constraints_error =
lifetime may not live long enough

Expand Down
8 changes: 8 additions & 0 deletions compiler/rustc_borrowck/src/session_diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -479,3 +479,11 @@ pub(crate) struct SimdIntrinsicArgConst {
pub arg: usize,
pub intrinsic: String,
}

#[derive(Diagnostic)]
#[diag(borrowck_intrinsic_const_vector_arg_non_const)]
pub(crate) struct IntrinsicConstVectorArgNonConst {
#[primary_span]
pub span: Span,
pub index: u128,
}
31 changes: 30 additions & 1 deletion compiler/rustc_borrowck/src/type_check/mod.rs
Copy link
Member

Choose a reason for hiding this comment

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

This diff is much bigger than needed. See #121225 for a PR that makes some other intrinsics require a constant argument; this should share that same check.

Copy link
Member

Choose a reason for hiding this comment

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

In fact the even better alternative is probably to remove the simd_shuffle case entirely, and just add the new attribute to simd_shuffle.

Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,9 @@ use rustc_mir_dataflow::move_paths::MoveData;
use rustc_mir_dataflow::ResultsCursor;

use crate::renumber::RegionCtxt;
use crate::session_diagnostics::{MoveUnsized, SimdIntrinsicArgConst};
use crate::session_diagnostics::{
IntrinsicConstVectorArgNonConst, MoveUnsized, SimdIntrinsicArgConst,
};
use crate::{
borrow_set::BorrowSet,
constraints::{OutlivesConstraint, OutlivesConstraintSet},
Expand Down Expand Up @@ -1630,6 +1632,33 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> {
intrinsic: name.to_string(),
});
}
} else if let Some(attr) =
self.tcx().get_attr(def_id, sym::rustc_intrinsic_const_vector_arg)
{
match attr.meta_item_list() {
Some(items) => {
items.into_iter().for_each(|item| {
if let Some(rustc_ast::LitKind::Int(index, _)) =
item.lit().map(|lit| &lit.kind)
{
if let Some(arg) = args.get::<usize>(index.get() as usize) {
if !matches!(arg, Spanned { node: Operand::Constant(_), .. }) {
self.tcx().dcx().emit_err(
IntrinsicConstVectorArgNonConst {
span: term.source_info.span,
index: index.get(),
},
);
}
}
} else {
span_mirbug!(self, term, "literal kind");
}
});
}
None => (),
}
// Error is reported by `rustc_attr!`
}
}
debug!(?func_ty);
Expand Down
5 changes: 5 additions & 0 deletions compiler/rustc_codegen_gcc/src/common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,11 @@ impl<'gcc, 'tcx> ConstMethods<'tcx> for CodegenCx<'gcc, 'tcx> {
self.context.new_struct_constructor(None, struct_type.as_type(), None, values)
}

fn const_vector(&self, values: &[RValue<'gcc>]) -> RValue<'gcc> {
let typ = self.type_vector(values[0].get_type(), values.len() as u64);
self.context.new_rvalue_from_vector(None, typ, values)
}

fn const_to_opt_uint(&self, _v: RValue<'gcc>) -> Option<u64> {
// TODO(antoyo)
None
Expand Down
4 changes: 4 additions & 0 deletions compiler/rustc_codegen_llvm/src/common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,10 @@ impl<'ll, 'tcx> ConstMethods<'tcx> for CodegenCx<'ll, 'tcx> {
struct_in_context(self.llcx, elts, packed)
}

fn const_vector(&self, elts: &[&'ll Value]) -> &'ll Value {
unsafe { llvm::LLVMConstVector(elts.as_ptr(), elts.len() as c_uint) }
}

fn const_to_opt_uint(&self, v: &'ll Value) -> Option<u64> {
try_as_const_integral(v).and_then(|v| unsafe {
let mut i = 0u64;
Expand Down
5 changes: 5 additions & 0 deletions compiler/rustc_codegen_ssa/messages.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ codegen_ssa_check_installed_visual_studio = please ensure that Visual Studio 201
codegen_ssa_compiler_builtins_cannot_call =
`compiler_builtins` cannot call functions through upstream monomorphizations; encountered invalid call from `{$caller}` to `{$callee}`

codegen_ssa_const_vector_evaluation = could not evaluate constant vector at compile time

codegen_ssa_copy_path = could not copy {$from} to {$to}: {$error}

codegen_ssa_copy_path_buf = unable to copy {$source_file} to {$output_path}: {$error}
Expand All @@ -31,6 +33,7 @@ codegen_ssa_expected_used_symbol = expected `used`, `used(compiler)` or `used(li

codegen_ssa_extern_funcs_not_found = some `extern` functions couldn't be found; some native libraries may need to be installed or have their path specified


codegen_ssa_extract_bundled_libs_archive_member = failed to get data from archive member '{$rlib}': {$error}
codegen_ssa_extract_bundled_libs_convert_name = failed to convert name '{$rlib}': {$error}
codegen_ssa_extract_bundled_libs_mmap_file = failed to mmap file '{$rlib}': {$error}
Expand Down Expand Up @@ -60,6 +63,8 @@ codegen_ssa_incorrect_cgu_reuse_type =

codegen_ssa_insufficient_vs_code_product = VS Code is a different product, and is not sufficient.

codegen_ssa_invalid_intrinsic_const_vector_arg = invalid argument, must be integer literal

codegen_ssa_invalid_link_ordinal_nargs = incorrect number of arguments to `#[link_ordinal]`
.note = the attribute requires exactly one argument

Expand Down
22 changes: 21 additions & 1 deletion compiler/rustc_codegen_ssa/src/codegen_attrs.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use rustc_ast::{ast, attr, MetaItemKind, NestedMetaItem};
use rustc_ast::{ast, attr, LitKind, MetaItemKind, MetaItemLit, NestedMetaItem};
use rustc_attr::{list_contains_name, InlineAttr, InstructionSetAttr, OptimizeAttr};
use rustc_errors::{codes::*, struct_span_code_err, DiagMessage, SubdiagMessage};
use rustc_hir as hir;
Expand Down Expand Up @@ -523,6 +523,26 @@ fn codegen_fn_attrs(tcx: TyCtxt<'_>, did: LocalDefId) -> CodegenFnAttrs {
))
})
}
sym::rustc_intrinsic_const_vector_arg => {
let mut indicies: Vec<usize> = Vec::new();
if let Some(item_list) = attr.meta_item_list() {
for item in item_list {
match item {
NestedMetaItem::Lit(MetaItemLit {
kind: LitKind::Int(index, _),
..
}) => {
indicies.push(index.get() as usize);
}
_ => {
tcx.dcx()
.emit_err(errors::IntrinsicConstVectorArg { span: attr.span });
}
}
}
}
codegen_fn_attrs.const_vector_indices = Some(indicies);
}
_ => {}
}
}
Expand Down
14 changes: 14 additions & 0 deletions compiler/rustc_codegen_ssa/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -598,6 +598,20 @@ pub struct ShuffleIndicesEvaluation {
pub span: Span,
}

#[derive(Diagnostic)]
#[diag(codegen_ssa_const_vector_evaluation)]
pub struct ConstVectorEvaluation {
#[primary_span]
pub span: Span,
}

#[derive(Diagnostic)]
#[diag(codegen_ssa_invalid_intrinsic_const_vector_arg)]
pub struct IntrinsicConstVectorArg {
#[primary_span]
pub span: Span,
}

#[derive(Diagnostic)]
#[diag(codegen_ssa_missing_memory_ordering)]
pub struct MissingMemoryOrdering;
Expand Down
32 changes: 28 additions & 4 deletions compiler/rustc_codegen_ssa/src/mir/block.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use super::{CachedLlbb, FunctionCx, LocalRef};

use crate::base::{self, is_call_from_compiler_builtins_to_upstream_monomorphization};
use crate::common::{self, IntPredicate};
use crate::errors::CompilerBuiltinsCannotCall;
use crate::errors;
use crate::meth;
use crate::traits::*;
use crate::MemFlags;
Expand Down Expand Up @@ -169,7 +169,7 @@ impl<'a, 'tcx> TerminatorCodegenHelper<'tcx> {
if destination.is_some() {
let caller = with_no_trimmed_paths!(tcx.def_path_str(fx.instance.def_id()));
let callee = with_no_trimmed_paths!(tcx.def_path_str(instance.def_id()));
tcx.dcx().emit_err(CompilerBuiltinsCannotCall { caller, callee });
tcx.dcx().emit_err(errors::CompilerBuiltinsCannotCall { caller, callee });
} else {
info!(
"compiler_builtins call to diverging function {:?} replaced with abort",
Expand Down Expand Up @@ -925,7 +925,8 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
// checked by the type-checker.
if i == 2 && intrinsic.name == sym::simd_shuffle {
if let mir::Operand::Constant(constant) = &arg.node {
let (llval, ty) = self.simd_shuffle_indices(bx, constant);
let (llval, ty) = self.early_evaluate_const_vector(bx, constant);

return OperandRef {
val: Immediate(llval),
layout: bx.layout_of(ty),
Expand Down Expand Up @@ -1003,9 +1004,32 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
(args, None)
};

let const_vec_arg_indexes = if let Some(def) = def {
let val = bx.tcx().codegen_fn_attrs(def.def_id());
let const_vector_indices = &val.const_vector_indices;
if let Some(const_vector_i) = const_vector_indices {
&const_vector_i.as_ref()
} else {
&Vec::<usize>::new()
}
} else {
&Vec::<usize>::new()
};

let mut copied_constant_arguments = vec![];
'make_args: for (i, arg) in first_args.iter().enumerate() {
let mut op = self.codegen_operand(bx, &arg.node);
let mut op = if const_vec_arg_indexes.contains(&i) {
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 you can just remove this if. It's always correct to use this translation if the value happens to be a constant SIMD vector, right?

In fact, should we maybe do this in OperandRef::from_const? Then it just happens for all constant SIMD vectors? That would in fact make the attribute entirely optional -- a nice-to-have to prevent LLVM errors, but not actually required to unblock stdarch.

if let mir::Operand::Constant(constant) = &arg.node
&& constant.ty().is_simd()
{
let (llval, ty) = self.early_evaluate_const_vector(bx, &constant);
Copy link
Member

Choose a reason for hiding this comment

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

So is there no LLVM intrinsic that needs a constant integer? It's only ever vectors? That seems odd.

OperandRef { val: Immediate(llval), layout: bx.layout_of(ty) }
} else {
span_bug!(span, "argument at {i} must be a constant vector");
}
} else {
self.codegen_operand(bx, &arg.node)
};

if let (0, Some(ty::InstanceKind::Virtual(_, idx))) = (i, def) {
match op.val {
Expand Down
81 changes: 62 additions & 19 deletions compiler/rustc_codegen_ssa/src/mir/constant.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ use crate::traits::*;
use rustc_middle::mir;
use rustc_middle::mir::interpret::ErrorHandled;
use rustc_middle::ty::layout::HasTyCtxt;
use rustc_middle::ty::ValTree;
use rustc_middle::ty::{self, Ty};
use rustc_middle::{bug, span_bug};
use rustc_target::abi::Abi;
Expand All @@ -29,7 +30,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
.expect("erroneous constant missed by mono item collection")
}

/// This is a convenience helper for `simd_shuffle_indices`. It has the precondition
/// This is a convenience helper for `early_evaluate_const_vector`. It has the precondition
/// that the given `constant` is an `Const::Unevaluated` and must be convertible to
/// a `ValTree`. If you want a more general version of this, talk to `wg-const-eval` on zulip.
///
Expand Down Expand Up @@ -60,36 +61,48 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
self.cx.tcx().const_eval_resolve_for_typeck(ty::ParamEnv::reveal_all(), uv, constant.span)
}

/// process constant containing SIMD shuffle indices
pub fn simd_shuffle_indices(
/// process constant SIMD vector or constant containing SIMD shuffle indices
pub fn early_evaluate_const_vector(
Copy link
Member

Choose a reason for hiding this comment

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

I find "early" here quite confusing, I don't think we otherwise use this adjective to refer to compile-time evaluation. Why not just evaluate_const_vector?

&mut self,
bx: &Bx,
constant: &mir::ConstOperand<'tcx>,
) -> (Bx::Value, Ty<'tcx>) {
let ty = self.monomorphize(constant.ty());
let ty_is_simd = ty.is_simd();
let field_ty = if ty_is_simd {
ty.simd_size_and_type(bx.tcx()).1
} else {
ty.builtin_index().unwrap()
};
let val = self
.eval_unevaluated_mir_constant_to_valtree(constant)
.ok()
.map(|x| x.ok())
.flatten()
.map(|val| {
let field_ty = ty.builtin_index().unwrap();
let values: Vec<_> = val
.unwrap_branch()
.iter()
.map(|field| {
if let Some(prim) = field.try_to_scalar() {
let layout = bx.layout_of(field_ty);
let Abi::Scalar(scalar) = layout.abi else {
bug!("from_const: invalid ByVal layout: {:#?}", layout);
};
bx.scalar_to_backend(prim, scalar, bx.immediate_backend_type(layout))
} else {
bug!("simd shuffle field {:?}", field)
let mut values: Vec<Bx::Value> = Vec::new();
// For reliably being able to handle either:
// pub struct defn(i32, i32)
// call(1, 0);
// OR
// pub struct defn([i32, 2])
// call([1, 0]);
//
// And outputting: @call(<2 x i32> <i32 0, i32 1>)
// thus treating them the same if they are a const vector
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 understand any part of this comment. What's call? Why does it not involve defn at all? Also, the code below needs comments explaining how it relates to these two cases.

But really, the biggest issue is that a nested loop makes no sense. There should be only one loop, the one that goes over all vector elements. The only difference between the two representations is that in one case we need one extra unwrap_branch()[0] since the "struct wrapping an array" adds one extra layer to the tree.

for field in val.unwrap_branch().iter() {
match field {
ValTree::Branch(_) => {
let scalars = self.flatten_branch_to_scalars(bx, &field, field_ty);
values.extend(scalars);
}
ValTree::Leaf(_) => {
let scalar = self.extract_scalar(bx, &field, field_ty);
values.push(scalar);
}
})
.collect();
bx.const_struct(&values, false)
}
}
if ty_is_simd { bx.const_vector(&values) } else { bx.const_struct(&values, false) }
})
.unwrap_or_else(|| {
bx.tcx().dcx().emit_err(errors::ShuffleIndicesEvaluation { span: constant.span });
Expand All @@ -99,4 +112,34 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
});
(val, ty)
}

fn flatten_branch_to_scalars(
&mut self,
bx: &Bx,
branch: &ValTree<'tcx>,
field_ty: Ty<'tcx>,
) -> Vec<Bx::Value> {
branch
.unwrap_branch()
.iter()
.map(|field| match field {
ValTree::Branch(_) => {
bug!("Cannot have arbitrarily nested const vectors: {:#?}", field)
}
ValTree::Leaf(_) => self.extract_scalar(bx, field, field_ty),
})
.collect()
}

fn extract_scalar(&mut self, bx: &Bx, field: &ValTree<'tcx>, field_ty: Ty<'tcx>) -> Bx::Value {
if let Some(prim) = field.try_to_scalar() {
let layout = bx.layout_of(field_ty);
let Abi::Scalar(scalar) = layout.abi else {
bug!("from_const: invalid ByVal layout: {:#?}", layout);
};
bx.scalar_to_backend(prim, scalar, bx.immediate_backend_type(layout))
} else {
bug!("field is not a scalar {:?}", field)
}
}
}
1 change: 1 addition & 0 deletions compiler/rustc_codegen_ssa/src/traits/consts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ pub trait ConstMethods<'tcx>: BackendTypes {

fn const_str(&self, s: &str) -> (Self::Value, Self::Value);
fn const_struct(&self, elts: &[Self::Value], packed: bool) -> Self::Value;
fn const_vector(&self, elts: &[Self::Value]) -> Self::Value;

fn const_to_opt_uint(&self, v: Self::Value) -> Option<u64>;
fn const_to_opt_u128(&self, v: Self::Value, sign_ext: bool) -> Option<u128>;
Expand Down
3 changes: 3 additions & 0 deletions compiler/rustc_feature/src/builtin_attrs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -837,6 +837,9 @@ pub const BUILTIN_ATTRIBUTES: &[BuiltinAttribute] = &[
rustc_runtime, Normal, template!(Word), WarnFollowing,
EncodeCrossCrate::No, INTERNAL_UNSTABLE
),
rustc_attr!(
rustc_intrinsic_const_vector_arg, Normal, template!(List: "arg1_index, arg2_index, ..."), ErrorFollowing, EncodeCrossCrate::Yes, INTERNAL_UNSTABLE
),

// ==========================================================================
// Internal attributes, Layout related:
Expand Down
Loading
Loading