Skip to content

Commit 021310a

Browse files
committed
Removed some (likely premature) optimizations to improve code clarity
1 parent 73dbf78 commit 021310a

File tree

2 files changed

+62
-142
lines changed
  • compiler/rustc_trait_selection/src/solve/eval_ctxt/canonical/dedup_solver

2 files changed

+62
-142
lines changed

compiler/rustc_trait_selection/src/solve/eval_ctxt/canonical/dedup_solver/solver.rs

Lines changed: 53 additions & 142 deletions
Original file line numberDiff line numberDiff line change
@@ -20,10 +20,8 @@ pub struct DedupSolver {
2020
/// The variables present in each constraint - the inner vec contains the variables, in the order
2121
/// that they appear in the constraint. See solver/tests.rs for examples on how constraints are lowered to this format
2222
constraint_vars: IndexVec<ConstraintIndex, Vec<VarIndex>>,
23-
/// The cliques that constraints are partitioned into. These are determined as follows: imagine a graph where
24-
/// each constraint is a vertex, and an edge exists between a pair of constraints if there is a many-to-one mapping of
25-
/// variables that perfectly maps one constraint onto the other. The constraint cliques are just cliques within this graph.
26-
/// By nature of this problem, it is impossible for a constraint to be in two cliques
23+
/// The cliques that constraints are partitioned into. Constraints can only be merged if they belong to the same clique,
24+
/// and it's impossible for a constraint to be in more than one clique
2725
constraint_cliques: IndexVec<CliqueIndex, Vec<ConstraintIndex>>,
2826
/// A set of variables we cannot remove, i.e. they belong to a universe that the caller can name. We keep track of these
2927
/// to determine if there's a variable that we **can** remove that behaves like one of these, where in that case we just
@@ -61,7 +59,8 @@ struct MappingInfo {
6159
dependencies: FxIndexMap<ConstraintIndex, BTreeSet<MappingIndex>>,
6260
}
6361
#[derive(Debug, PartialEq, Eq)]
64-
enum MapEvalErr {
62+
enum MapEval {
63+
Ok(MappingInfo),
6564
Conflicts,
6665
Unremovable,
6766
}
@@ -79,125 +78,13 @@ impl DedupSolver {
7978

8079
mappings: FxIndexMap::default(),
8180
removed_constraints: RefCell::new(FxIndexSet::default()),
82-
applied_mappings: RefCell::new(Mapping::from(&[], &[])),
81+
applied_mappings: RefCell::new(Mapping::from(&[], &[]).unwrap()),
8382
};
84-
deduper.refine_cliques();
8583
deduper.compute_mappings();
8684
deduper.resolve_dependencies();
8785

8886
DedupResult { removed_constraints: deduper.removed_constraints.into_inner() }
8987
}
90-
/// The input cliques are provided just on a basis of the structure of the constraints, i.e.
91-
/// "are they the same if we ignore variables unnameable from the caller". However, just because
92-
/// this is the case doesn't mean that two constraints can be merged - for example, the constraint
93-
/// involving vars [1, 3] can't be merged with a constraint involving vars [2, 2].
94-
/// This function refines the cliques such that if we create a graph with constraints as vertices
95-
/// and edges if they can be merged, constraint_cliques now represents the **true** cliques of the
96-
/// graph, i.e. any two constrains in the same clique can now create a valid mapping
97-
fn refine_cliques(&mut self) {
98-
// Refine categories based on shape - see canonicalize_constraint_shape for more info
99-
for clique_indx in (0..self.constraint_cliques.len()).map(CliqueIndex::new) {
100-
let mut shape_cliques: FxIndexMap<Vec<usize>, CliqueIndex> = FxIndexMap::default();
101-
let mut constraint_indx = 0;
102-
while constraint_indx < self.constraint_cliques[clique_indx].len() {
103-
let constraint = self.constraint_cliques[clique_indx][constraint_indx];
104-
let shape =
105-
Self::canonicalize_constraint_shape(&mut self.constraint_vars[constraint]);
106-
let is_first_entry = shape_cliques.is_empty();
107-
let new_clique = *shape_cliques.entry(shape).or_insert_with(|| {
108-
if is_first_entry {
109-
clique_indx
110-
} else {
111-
self.constraint_cliques.push(Vec::new());
112-
CliqueIndex::new(self.constraint_cliques.len() - 1)
113-
}
114-
});
115-
if new_clique == clique_indx {
116-
constraint_indx += 1;
117-
continue;
118-
}
119-
self.constraint_cliques[clique_indx].swap_remove(constraint_indx);
120-
self.constraint_cliques[new_clique].push(constraint);
121-
}
122-
}
123-
// Refine categories based on indices of variables. This is based on the observation that
124-
// if a variable V is present in a constraint C1 at some set of indices I, then a constraint
125-
// C2 can be merged with C1 only if one of the following cases are satisfied:
126-
// a. V is present in constraint C2 at the **same** set of indices I, where in that case
127-
// the variable mapping that merges these two constraints would just map V onto V
128-
// b. V is not present in constraint C2 at all, in which case some other variable would
129-
// be mapped onto V
130-
// If none of these above cases are true, that means we have a situation where we map V
131-
// to another variable U, and a variable W would be mapped onto V - in this case, we're just
132-
// shuffling variables around without actually eliminating any, which is unproductive and
133-
// hence an "invalid mapping"
134-
for clique_indx in (0..self.constraint_cliques.len()).map(CliqueIndex::new) {
135-
// First element of tuple (the FxIndexMap) maps a variable to
136-
// the index it occurs in
137-
let mut index_cliques: Vec<(FxIndexMap<VarIndex, usize>, CliqueIndex)> = Vec::new();
138-
let mut constraint_indx = 0;
139-
while constraint_indx < self.constraint_cliques[clique_indx].len() {
140-
let constraint = self.constraint_cliques[clique_indx][constraint_indx];
141-
let constraint_vars = &self.constraint_vars[constraint];
142-
let constraint_var_indices: FxIndexMap<VarIndex, usize> =
143-
constraint_vars.iter().enumerate().map(|(indx, x)| (*x, indx)).collect();
144-
145-
let mut found_clique = None;
146-
for (clique_vars, new_clique_indx) in index_cliques.iter_mut() {
147-
let is_clique_member = constraint_vars
148-
.iter()
149-
.enumerate()
150-
.all(|(indx, x)| *clique_vars.get(x).unwrap_or(&indx) == indx);
151-
if !is_clique_member {
152-
continue;
153-
}
154-
found_clique = Some(*new_clique_indx);
155-
clique_vars.extend(&constraint_var_indices);
156-
break;
157-
}
158-
let new_clique = found_clique.unwrap_or_else(|| {
159-
if index_cliques.is_empty() {
160-
clique_indx
161-
} else {
162-
let new_clique = self.constraint_cliques.next_index();
163-
self.constraint_cliques.push(Vec::new());
164-
index_cliques.push((constraint_var_indices, new_clique));
165-
new_clique
166-
}
167-
});
168-
if new_clique == clique_indx {
169-
constraint_indx += 1;
170-
continue;
171-
}
172-
self.constraint_cliques[clique_indx].swap_remove(constraint_indx);
173-
self.constraint_cliques[new_clique].push(constraint);
174-
}
175-
}
176-
}
177-
/// Returns the "shape" of a constraint, which captures information about the location(s) and
178-
/// multiplicity of variables in the constraint, irrespective of the actual variable indices
179-
/// For example, a constraint involving the vars [1, 1, 2, 3] has a shape of [0, 0, 1, 2],
180-
/// and a constraint involving vars [3, 4] has a shape of [0, 1]
181-
/// It takes a mutable reference to the vars because it also removes duplicates from
182-
/// the input vector after computing the shape
183-
/// Clearly, two constraints can be mapped onto each other only if they have the
184-
/// same shape
185-
fn canonicalize_constraint_shape(vars: &mut Vec<VarIndex>) -> Vec<usize> {
186-
let mut shape = Vec::new();
187-
let mut num_vars = 0;
188-
let mut indx = 0;
189-
while indx < vars.len() {
190-
if let Some(val) = shape.iter().find(|y| vars[**y] == vars[indx]) {
191-
shape.push(*val);
192-
vars.remove(indx);
193-
} else {
194-
shape.push(num_vars);
195-
num_vars += 1;
196-
indx += 1;
197-
}
198-
}
199-
shape
200-
}
20188

20289
/// Computes the set of all possible mappings
20390
/// If a mapping has no dependencies, then it's eagerly taken, to increase performance
@@ -211,27 +98,22 @@ impl DedupSolver {
21198
.enumerate()
21299
.filter(|x| !self.removed_constraints.borrow().contains(x.1))
213100
{
101+
let constraint_1_vars = &self.constraint_vars[*constraint_1];
214102
for constraint_2 in clique
215103
.iter()
216104
.skip(n + 1)
217105
.filter(|x| !self.removed_constraints.borrow().contains(*x))
218106
{
107+
let constraint_2_vars = &self.constraint_vars[*constraint_2];
219108
// Maps constraint_1 to constraint_2
220-
let forward = Mapping::from(
221-
&self.constraint_vars[*constraint_1],
222-
&self.constraint_vars[*constraint_2],
223-
);
109+
let forward = Mapping::from(constraint_1_vars, constraint_2_vars);
224110
// Maps constraint_2 to constraint_1
225-
let reverse = Mapping::from(
226-
&self.constraint_vars[*constraint_2],
227-
&self.constraint_vars[*constraint_1],
228-
);
229-
if self.mappings.contains_key(&forward) || self.mappings.contains_key(&reverse)
230-
{
111+
let reverse = Mapping::from(constraint_2_vars, constraint_1_vars);
112+
let (Ok(forward), Ok(reverse)) = (forward, reverse) else {
231113
continue;
232-
}
114+
};
233115

234-
// if constraint_1 and constraint_2 can be merged, this relation should be
116+
// If constraint_1 and constraint_2 can be merged, this relation should be
235117
// bidirectional, i.e. we can merge 1 into 2 or 2 into 1
236118
// For example, if a clique contains constraints [1, 2] and [11, 12] and another
237119
// clique contains constraint [1], then we **cannot** merge vars 1 and 11 - the
@@ -240,17 +122,15 @@ impl DedupSolver {
240122
// map the constraint [1] into [11], which is a constraint that doesn't exist
241123
let (eval_forward, eval_reverse) =
242124
(self.eval_mapping(&forward), self.eval_mapping(&reverse));
243-
if eval_forward == Err(MapEvalErr::Conflicts)
244-
|| eval_reverse == Err(MapEvalErr::Conflicts)
245-
{
125+
if eval_forward == MapEval::Conflicts || eval_reverse == MapEval::Conflicts {
246126
continue;
247127
}
248-
if let Ok(eval_forward) = eval_forward {
128+
if let MapEval::Ok(eval_forward) = eval_forward {
249129
if self.try_apply_mapping(&forward, &eval_forward, false) == Err(true) {
250130
self.mappings.insert(forward, eval_forward);
251131
}
252132
}
253-
if let Ok(eval_reverse) = eval_reverse {
133+
if let MapEval::Ok(eval_reverse) = eval_reverse {
254134
if self.try_apply_mapping(&reverse, &eval_reverse, false) == Err(true) {
255135
self.mappings.insert(reverse, eval_reverse);
256136
}
@@ -266,7 +146,7 @@ impl DedupSolver {
266146
/// MappingInfo can contain dependencies - these occur if a mapping *partially* maps
267147
/// a constraint onto another, so the mapping isn't immediately invalid, but we do need
268148
/// another mapping to complete that partial map for it to actually be valid
269-
fn eval_mapping(&self, mapping: &Mapping) -> Result<MappingInfo, MapEvalErr> {
149+
fn eval_mapping(&self, mapping: &Mapping) -> MapEval {
270150
let maps_unremovable_var =
271151
mapping.0.iter().any(|(from, to)| self.unremovable_vars.contains(from) && from != to);
272152

@@ -280,7 +160,9 @@ impl DedupSolver {
280160
let mut found_non_conflicting = false;
281161
for constraint_2 in clique.iter() {
282162
let vars_2 = &self.constraint_vars[*constraint_2];
283-
let trial_mapping = Mapping::from(vars_1, vars_2);
163+
let Ok(trial_mapping) = Mapping::from(vars_1, vars_2) else {
164+
continue;
165+
};
284166
if mapping.conflicts_with(&trial_mapping) {
285167
continue;
286168
}
@@ -297,14 +179,14 @@ impl DedupSolver {
297179
}
298180
}
299181
if !found_non_conflicting {
300-
return Err(MapEvalErr::Conflicts);
182+
return MapEval::Conflicts;
301183
}
302184
}
303185
}
304186
if maps_unremovable_var {
305-
return Err(MapEvalErr::Unremovable);
187+
return MapEval::Unremovable;
306188
}
307-
Ok(info)
189+
MapEval::Ok(info)
308190
}
309191
/// Currently, dependencies are in the form FxIndexMap<ConstraintIndex, Empty FxIndexSet>,
310192
/// where ConstraintIndex is the constraint we must *also* map in order to apply this mapping.
@@ -494,22 +376,51 @@ impl DedupSolver {
494376
}
495377

496378
impl Mapping {
497-
fn from(from: &[VarIndex], to: &[VarIndex]) -> Self {
498-
Self(from.iter().zip(to).map(|(x, y)| (*x, *y)).collect())
379+
/// Creates a mapping between two constraints. If the resulting mapping is invalid,
380+
/// an Err is returned
381+
fn from(from: &[VarIndex], to: &[VarIndex]) -> Result<Self, ()> {
382+
if from.len() != to.len() {
383+
return Err(());
384+
}
385+
let mut mapping_set = BTreeMap::new();
386+
for (from_var, to_var) in from.iter().zip(to) {
387+
if let Some(previous_map) = mapping_set.get(from_var) {
388+
if previous_map != to_var {
389+
return Err(());
390+
}
391+
continue;
392+
}
393+
mapping_set.insert(*from_var, *to_var);
394+
}
395+
// We impose a constraint that a variable cannot be both a key and a value of
396+
// a mapping, as that would mean [1, 2] can be mapped onto [2, 1] - however,
397+
// these are fundamentally different constraints that can't be merged.
398+
// The only exception is if a var maps to itself - that's fine, as all it's saying
399+
// is that we want to fix a variable and don't map it
400+
if mapping_set.values().any(|x| mapping_set.get(x).unwrap_or(x) != x) {
401+
return Err(());
402+
}
403+
Ok(Self(mapping_set))
499404
}
500405
fn maps_var(&self, constraint: VarIndex) -> Option<VarIndex> {
501406
self.0.get(&constraint).map(|x| *x)
502407
}
408+
/// Returns whether the mapping will change the given constraint if applied
503409
fn affects_constraint(&self, constraint: &[VarIndex]) -> bool {
504410
constraint.iter().any(|x| self.maps_var(*x).unwrap_or(*x) != *x)
505411
}
412+
/// Returns whether a mapping is a superset of another mapping
506413
fn contains_fully(&self, other: &Self) -> bool {
507414
other.0.iter().all(|(from, to)| self.maps_var(*from) == Some(*to))
508415
}
416+
/// Returns whether a mapping conflicts with another mapping, i.e. they can't be applied together
509417
fn conflicts_with(&self, other: &Self) -> bool {
510418
for (from_a, to_a) in self.0.iter() {
511419
for (from_b, to_b) in other.0.iter() {
420+
// Maps the same key to different values - conflicts!
512421
let map_conflicts = from_a == from_b && to_a != to_b;
422+
// Map A maps var v to w, but map B maps w to v. Applying both maps together doesn't
423+
// remove any variables, but just shuffles them around, so we call this a conflict
513424
let not_productive =
514425
to_b == from_a && from_a != to_a || to_a == from_b && from_b != to_b;
515426
if map_conflicts || not_productive {

compiler/rustc_trait_selection/src/solve/eval_ctxt/canonical/dedup_solver/solver/tests.rs

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,15 @@ fn test_simple() {
6666
assert!([constraint_set([0]), constraint_set([1])].contains(&deduped.removed_constraints));
6767
}
6868
#[test]
69+
fn test_simple_2() {
70+
let deduped = DedupSolver::dedup(
71+
constraint_vars(vec![vec![0, 1], vec![2, 1], vec![1, 0], vec![1, 2]]),
72+
constraint_cliques(vec![vec![0, 1, 2, 3]]),
73+
FxIndexSet::from_iter([VarIndex::new(0), VarIndex::new(1)]),
74+
);
75+
assert_eq!(deduped.removed_constraints, constraint_set([1, 3]));
76+
}
77+
#[test]
6978
fn test_dependencies() {
7079
// Example of constraint that produces this lowering:
7180
// (&'?1 Foo, &'?2 Foo): &'?13

0 commit comments

Comments
 (0)