Skip to content

Commit e3ecc57

Browse files
eddybeddyb
committed
builder: always pointercast first before attempting to merge OpAccessChains.
1 parent d0686a2 commit e3ecc57

File tree

2 files changed

+70
-36
lines changed

2 files changed

+70
-36
lines changed

‎crates/rustc_codegen_spirv/src/builder/builder_methods.rs‎

Lines changed: 36 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -854,12 +854,34 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
854854

855855
// --- End Recovery Path ---
856856

857+
// FIXME(eddyb) the comments below might not make much sense, because this
858+
// used to be in the "fallback path" before being moved to before merging.
859+
//
860+
// Before emitting the AccessChain, explicitly cast the base pointer `ptr` to
861+
// ensure its pointee type matches the input `ty`. This is required because the
862+
// SPIR-V `AccessChain` instruction implicitly uses the size of the base
863+
// pointer's pointee type when applying the *first* index operand (our
864+
// `ptr_base_index`). If `ty` and `original_pointee_ty` mismatched and we
865+
// reached this fallback, this cast ensures SPIR-V validity.
866+
trace!("maybe_inbounds_gep fallback path calling pointercast");
867+
// Cast ptr to point to `ty`.
868+
// HACK(eddyb) temporary workaround for untyped pointers upstream.
869+
// FIXME(eddyb) replace with untyped memory SPIR-V + `qptr` or similar.
870+
let ptr = self.pointercast(ptr, self.type_ptr_to(ty));
871+
// Get the ID of the (potentially newly casted) pointer.
872+
let ptr_id = ptr.def(self);
873+
// HACK(eddyb) updated pointee type of `ptr` post-`pointercast`.
874+
let original_pointee_ty = ty;
875+
857876
// --- Attempt GEP Merging Path ---
858877

859878
// Check if the base pointer `ptr` itself was the result of a previous
860879
// AccessChain instruction. Merging is only attempted if the input type `ty`
861880
// matches the pointer's actual underlying pointee type `original_pointee_ty`.
862881
// If they differ, merging could be invalid.
882+
// HACK(eddyb) always attempted now, because we `pointercast` first, which:
883+
// - is noop when `ty == original_pointee_ty` pre-`pointercast` (old condition)
884+
// - may generate (potentially mergeable) new `AccessChain`s in other cases
863885
let maybe_original_access_chain = if ty == original_pointee_ty {
864886
// Search the current function's instructions...
865887
// FIXME(eddyb) this could get ridiculously expensive, at the very least
@@ -908,12 +930,21 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
908930
// 2. The *last* index of the original AccessChain is a constant.
909931
// 3. The *first* index (`ptr_base_index`) of the *current* GEP is a constant.
910932
// Merging usually involves adding these two constant indices.
933+
//
934+
// FIXME(eddyb) the above comment seems inaccurate, there is no reason
935+
// why runtime indices couldn't be added together just like constants
936+
// (and in fact this is needed nowadays for all array indexing).
911937
let can_merge = if let Some(&last_original_idx_id) = original_indices.last() {
912-
// Check if both the last original index and the current base index are constant scalars.
913-
self.builder
914-
.lookup_const_scalar(last_original_idx_id.with_type(ptr_base_index.ty))
915-
.is_some()
916-
&& self.builder.lookup_const_scalar(ptr_base_index).is_some()
938+
// HACK(eddyb) see the above comment, this bypasses the const
939+
// check below, without tripping a clippy warning etc.
940+
let always_merge = true;
941+
always_merge || {
942+
// Check if both the last original index and the current base index are constant scalars.
943+
self.builder
944+
.lookup_const_scalar(last_original_idx_id.with_type(ptr_base_index.ty))
945+
.is_some()
946+
&& self.builder.lookup_const_scalar(ptr_base_index).is_some()
947+
}
917948
} else {
918949
// Original access chain had no indices to merge with.
919950
false
@@ -966,21 +997,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
966997
// This path is taken if neither the Recovery nor the Merging path succeeded or applied.
967998
// It performs a more direct translation of the GEP request.
968999

969-
// HACK(eddyb): Workaround for potential upstream issues where pointers might lack precise type info.
970-
// FIXME(eddyb): Ideally, this should use untyped memory features if available/necessary.
971-
972-
// Before emitting the AccessChain, explicitly cast the base pointer `ptr` to
973-
// ensure its pointee type matches the input `ty`. This is required because the
974-
// SPIR-V `AccessChain` instruction implicitly uses the size of the base
975-
// pointer's pointee type when applying the *first* index operand (our
976-
// `ptr_base_index`). If `ty` and `original_pointee_ty` mismatched and we
977-
// reached this fallback, this cast ensures SPIR-V validity.
978-
trace!("maybe_inbounds_gep fallback path calling pointercast");
979-
// Cast ptr to point to `ty`.
980-
let ptr = self.pointercast(ptr, self.type_ptr_to(ty));
981-
// Get the ID of the (potentially newly casted) pointer.
982-
let ptr_id = ptr.def(self);
983-
9841000
trace!(
9851001
"emitting access chain via fallback path with pointer type: {}",
9861002
self.debug_type(final_spirv_ptr_type)

‎tests/compiletests/ui/dis/panic_builtin_bounds_check.stderr‎

Lines changed: 34 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -6,27 +6,45 @@ OpEntryPoint Fragment %2 "main"
66
OpExecutionMode %2 OriginUpperLeft
77
%3 = OpString "/n[Rust panicked at $DIR/panic_builtin_bounds_check.rs:25:5]/n index out of bounds: the len is %u but the index is %u/n in main()/n"
88
%4 = OpString "$DIR/panic_builtin_bounds_check.rs"
9-
%5 = OpTypeVoid
10-
%6 = OpTypeFunction %5
11-
%7 = OpTypeBool
9+
OpDecorate %5 ArrayStride 4
10+
%6 = OpTypeVoid
11+
%7 = OpTypeFunction %6
1212
%8 = OpTypeInt 32 0
13-
%9 = OpConstant %8 5
14-
%10 = OpConstant %8 4
15-
%11 = OpUndef %8
16-
%2 = OpFunction %5 None %6
17-
%12 = OpLabel
13+
%9 = OpConstant %8 4
14+
%5 = OpTypeArray %8 %9
15+
%10 = OpTypePointer Function %5
16+
%11 = OpConstant %8 0
17+
%12 = OpConstant %8 1
18+
%13 = OpConstant %8 2
19+
%14 = OpConstant %8 3
20+
%15 = OpTypeBool
21+
%16 = OpConstant %8 5
22+
%17 = OpUndef %8
23+
%18 = OpTypePointer Function %8
24+
%2 = OpFunction %6 None %7
25+
%19 = OpLabel
26+
OpLine %4 30 4
27+
%20 = OpVariable %10 Function
28+
OpLine %4 30 23
29+
%21 = OpCompositeConstruct %5 %11 %12 %13 %14
1830
OpLine %4 25 4
19-
%13 = OpULessThan %7 %9 %10
31+
OpStore %20 %21
32+
%22 = OpULessThan %15 %16 %9
2033
OpNoLine
21-
OpSelectionMerge %14 None
22-
OpBranchConditional %13 %15 %16
23-
%15 = OpLabel
24-
OpBranch %14
25-
%16 = OpLabel
34+
OpSelectionMerge %23 None
35+
OpBranchConditional %22 %24 %25
36+
%24 = OpLabel
37+
OpBranch %23
38+
%25 = OpLabel
2639
OpLine %4 25 4
27-
%17 = OpExtInst %5 %1 1 %3 %11 %9
40+
%26 = OpExtInst %6 %1 1 %3 %17 %16
2841
OpNoLine
2942
OpReturn
30-
%14 = OpLabel
43+
%23 = OpLabel
44+
OpLine %4 25 4
45+
%27 = OpIAdd %8 %11 %16
46+
%28 = OpInBoundsAccessChain %18 %20 %27
47+
%29 = OpLoad %8 %28
48+
OpNoLine
3149
OpReturn
3250
OpFunctionEnd

0 commit comments

Comments
 (0)