Skip to content
Open
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
11 changes: 1 addition & 10 deletions crates/rustc_codegen_spirv/src/abi.rs
Original file line number Diff line number Diff line change
Expand Up @@ -92,15 +92,6 @@ pub(crate) fn provide(providers: &mut Providers) {
// <https://github.com/rust-lang/rust/commit/eaaa03faf77b157907894a4207d8378ecaec7b45>
arg.make_direct_deprecated();

// FIXME(eddyb) detect `#[rust_gpu::vector::v1]` more specifically,
// to avoid affecting anything should actually be passed as a pair.
if let PassMode::Pair(..) = arg.mode {
// HACK(eddyb) this avoids breaking e.g. `&[T]` pairs.
if let TyKind::Adt(..) = arg.layout.ty.kind() {
arg.mode = PassMode::Direct(ArgAttributes::new());
}
}

// Avoid pointlessly passing ZSTs, just like the official Rust ABI.
if arg.layout.is_zst() {
arg.mode = PassMode::Ignore;
Expand Down Expand Up @@ -490,7 +481,7 @@ pub fn scalar_pair_element_backend_type<'tcx>(
ty: TyAndLayout<'tcx>,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This reminds me that a longstanding refactor has been replacing all ty: TyAndLayout to layout: TyAndLayout (because it derefs to the layout, but not the type, so e.g. layout.ty and layout.size, instead of ty.ty and ty.size or the redundant ty.layout.size).

IIRC I regretted not naming it LayoutWithTy or similar, to make clearer the intent.

index: usize,
) -> Word {
let [a, b] = match ty.layout.backend_repr() {
let [a, b] = match ty.backend_repr {
BackendRepr::ScalarPair(a, b) => [a, b],
other => span_bug!(
span,
Expand Down
49 changes: 37 additions & 12 deletions crates/rustc_codegen_spirv/src/builder/format_args_decompiler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -557,35 +557,60 @@ impl<'tcx> DecodedFormatArgs<'tcx> {
if let Some((template_id, template_ty_id, rt_args_ptr_id, rt_args_ptr_ty_id)) =
split_fmt_args
{
let ctor = if let (Some(template_len), Some(rt_args_count)) = (
if let (Some(template_len), Some(rt_args_count)) = (
const_ptr_to_composite_len(template_id)
.or_else(|| array_len_from_ptr_type(template_ty_id)),
const_ptr_to_composite_len(rt_args_ptr_id)
.or_else(|| array_len_from_ptr_type(rt_args_ptr_ty_id)),
) {
FmtArgsCtor::NewTemplate {
template_len,
rt_args_count,
}
(
FmtArgsCtor::NewTemplate {
template_len,
rt_args_count,
},
SmallVec::<[Word; 8]>::from_slice(&[template_id, rt_args_ptr_id]),
)
} else if let Some(&[Inst::Call(_, callee_id, ref call_args)]) =
try_rev_take(-1).as_deref()
&& call_args.len() == 2
&& [call_args[0], call_args[1]] == [template_id, rt_args_ptr_id]
{
// Consume the matched call instruction.
try_rev_take(1).unwrap();
lookup_fmt_args_ctor(callee_id)?
(
lookup_fmt_args_ctor(callee_id)?,
SmallVec::<[Word; 8]>::from_slice(&[template_id, rt_args_ptr_id]),
)
} else if let Some(
&[
Inst::Call(call_ret_id, callee_id, ref call_args),
Inst::CompositeExtract(extracted0, from0, 0),
Inst::CompositeExtract(extracted1, from1, 1),
],
) = try_rev_take(-3).as_deref()
&& [from0, from1] == [call_ret_id; 2]
&& [extracted0, extracted1] == [template_id, rt_args_ptr_id]
{
// Newer rustc, since `BackendRepr::ScalarPair` args are no
// longer forced to `PassMode::Direct`, returns the whole
// `fmt::Arguments` from its `new_*` constructor as a scalar
// pair, and splits it (via `OpCompositeExtract`s) into the
// two scalar values passed to the panic entry-point.
//
// The constructor's own arguments (i.e. `pieces`/`template`
// and the `rt::Argument` slice pointers) still carry the
// recoverable const data, so use those, like the aggregate
// (non-split) `Call`+`extract`+`insert` case does below.
let call_args_storage = call_args.iter().copied().collect();
// Consume the matched call + both `OpCompositeExtract`s.
try_rev_take(3).unwrap();
(lookup_fmt_args_ctor(callee_id)?, call_args_storage)
Comment on lines +584 to +607

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The only reason I don't like this has to do with it feeling misplaced, likely a consequence of the split_fmt_args changes from months ago, which I might eventually revisit (and shouldn't block this PR).

} else {
// We failed to recover constructor metadata for an already-split
// `fmt::Arguments` value. Keep panic lowering sound by falling
// back to an unknown panic message, without requiring decompilation.
return Ok(decoded_format_args);
};

(
ctor,
SmallVec::<[Word; 8]>::from_slice(&[template_id, rt_args_ptr_id]),
)
}
} else {
// Newer rustc can pass the `fmt::Arguments::new_*` result directly to
// panic entry points (single trailing call), while older versions go
Expand Down
47 changes: 21 additions & 26 deletions crates/rustc_codegen_spirv/src/codegen_cx/entry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ use rspirv::spirv::{
BuiltIn, Decoration, Dim, ExecutionModel, FunctionControl, StorageClass, Word,
};
use rustc_abi::FieldsShape;
use rustc_codegen_ssa::mir::operand::{OperandRef, OperandValue};
use rustc_codegen_ssa::mir::place::PlaceRef;
use rustc_codegen_ssa::traits::{BaseTypeCodegenMethods, BuilderMethods, MiscCodegenMethods as _};
use rustc_data_structures::fx::FxHashMap;
use rustc_errors::MultiSpan;
Expand Down Expand Up @@ -87,22 +89,7 @@ impl<'tcx> CodegenCx<'tcx> {
};
for (arg_abi, hir_param) in fn_abi.args.iter().zip(hir_params) {
match arg_abi.mode {
PassMode::Direct(_) | PassMode::Ignore => {}
PassMode::Pair(..) => {
// FIXME(eddyb) implement `ScalarPair` `Input`s, or change
// the `FnAbi` readjustment to only use `PassMode::Pair` for
// pointers to `!Sized` types, but not other `ScalarPair`s.
if !matches!(arg_abi.layout.ty.kind(), ty::Ref(..)) {
self.tcx.dcx().span_err(
hir_param.ty_span,
format!(
"entry point parameter type not yet supported \
(`{}` has `ScalarPair` ABI but is not a `&T`)",
arg_abi.layout.ty
),
);
}
}
PassMode::Direct(_) | PassMode::Pair(..) | PassMode::Ignore => {}
_ => span_bug!(
hir_param.ty_span,
"query hooks should've made this `PassMode` impossible: {:#?}",
Expand Down Expand Up @@ -517,14 +504,6 @@ impl<'tcx> CodegenCx<'tcx> {
vs layout:\n{value_layout:#?}",
entry_arg_abi.layout.ty
);
if is_pair && !is_unsized {
// If PassMode is Pair, then we need to fill in the second part of the pair with a
// value. We currently only do that with unsized types, so if a type is a pair for some
// other reason (e.g. a tuple), we bail.
self.tcx
.dcx()
.span_fatal(hir_param.ty_span, "pair type not supported yet")
}
// FIXME(eddyb) should this talk about "typed buffers" instead of "interface blocks"?
// FIXME(eddyb) should we talk about "descriptor indexing" or
// actually use more reasonable terms like "resource arrays"?
Expand Down Expand Up @@ -647,8 +626,8 @@ impl<'tcx> CodegenCx<'tcx> {
}
}

let value_len = if is_pair {
// We've already emitted an error, fill in a placeholder value
let value_len = if is_pair && is_unsized {
// For wide references (e.g., slices), the second component is a length.
Comment on lines -650 to +630

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Losing the old comment makes this look unsound (undef is only being used because this situation is illegal).

Also, the variables make this additionally confusing, because is_... refer to two different aspects.

is_pair is "pass as pair" or "expects pair", whereas is_unsized should probably be renamed to pointee_is_unsized (it only really makes sense with refs, i.e. "ref to unsized").

Not to mention that the condition for "value_len is needed" is ty::Ref(..)+is_pair, with is_unsized being related but not necessarily the same thing.

Also, self.type_isize() isn't the right thing to use in the general case, and could lead to panics elsewhere if the unsized type is not a slice (though today that's only &dyn Trait, so I'm not sure it's testable at all).


Minimal fix: include the error justification in a comment, use scalar_pair_element_backend_type w/ entry_arg_abi.layout, instead of type_isize, maybe gate by ty::Ref instead of is_unsized.

Some(bx.undef(self.type_isize()))
} else {
None
Expand Down Expand Up @@ -693,6 +672,22 @@ impl<'tcx> CodegenCx<'tcx> {
call_args.push(value);
assert_eq!(value_len, None);
}
PassMode::Pair(..) => {
// Load both elements of the scalar pair from the input variable.
assert_eq!(storage_class, Ok(StorageClass::Input));
let OperandRef {
val: OperandValue::Pair(v0, v1),
..
} = bx.load_operand(PlaceRef::new_sized(
value_ptr.unwrap(),
entry_arg_abi.layout,
))
Comment on lines +681 to +684

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think load_operand should be used uniformly (in this else {...}), when storage_class is Ok, and then pattern-matching on (operand.val, entry_arg_abi.mode).

This would also force SpecConstants to require PassMode::Direct (right now they technically allow PassMode::Ignore, not sure if anything else catches that).

else {
unreachable!();
};
call_args.extend([v0, v1]);
assert_eq!(value_len, None);
}
_ => unreachable!(),
}
}
Expand Down
26 changes: 12 additions & 14 deletions tests/compiletests/ui/dis/complex_image_sample_inst.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -2,19 +2,17 @@
%4 = OpFunctionParameter %2
%5 = OpFunctionParameter %6
%7 = OpFunctionParameter %6
%8 = OpLabel
%9 = OpCompositeExtract %10 %5 0
%11 = OpCompositeExtract %10 %5 1
%12 = OpCompositeConstruct %6 %9 %11
%13 = OpCompositeExtract %10 %7 0
%14 = OpCompositeExtract %10 %7 1
%15 = OpCompositeConstruct %6 %13 %14
OpLine %16 29 13
%17 = OpAccessChain %18 %19 %20
OpLine %16 30 13
%21 = OpLoad %22 %17
OpLine %16 34 13
%23 = OpImageSampleProjExplicitLod %2 %21 %4 Grad %12 %15
%8 = OpFunctionParameter %6
%9 = OpFunctionParameter %6
%10 = OpLabel
%11 = OpCompositeConstruct %12 %5 %7
%13 = OpCompositeConstruct %12 %8 %9
OpLine %14 29 13
%15 = OpAccessChain %16 %17 %18
OpLine %14 30 13
%19 = OpLoad %20 %15
OpLine %14 34 13
%21 = OpImageSampleProjExplicitLod %2 %19 %4 Grad %11 %13
OpNoLine
OpReturnValue %23
OpReturnValue %21
OpFunctionEnd
27 changes: 27 additions & 0 deletions tests/compiletests/ui/lang/abi/scalar_pair.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
// build-pass
// compile-flags: -C target-feature=+Int64

use spirv_std::spirv;

#[spirv(fragment)]
pub fn main_future_proof(
#[spirv(flat)] input: (u64, u32),
out: &mut (u64, u32),
#[spirv(storage_buffer, descriptor_set = 0, binding = 0)] buffer_in: &(u64, u32),
#[spirv(storage_buffer, descriptor_set = 1, binding = 0)] buffer_out: &mut (u64, u32),
) {
*out = trans0(trans_ref(buffer_in));
*buffer_out = trans1(input);
}

pub fn trans0(arg: (u64, u32)) -> (u64, u32) {
(arg.0 + 1, arg.1 - 1)
}

pub fn trans1((a, b): (u64, u32)) -> (u64, u32) {
(a * 2, b * 3)
}

pub fn trans_ref((a, b): &(u64, u32)) -> (u64, u32) {
(a - 1, b - 1)
}
Loading