Determine range constraints from fixed lookup.

[chriseth]

No description provided.

[georgwiese]

Awesome! Took me while to understand but it does what I thought it would do.

[georgwiese]

I'm not sure I understand this case. In this case, no match was found right? So this branch is actually impossible?

[chriseth]

I mean yes, but it's not like more information about the inputs will help. As I wrote there, if we could express "invalid" in the range constraints, we should return that. We could also panic. I'll create an issue.

[chriseth]

-> #2324

[georgwiese]

I think that would be clearer. Or add a comment that intersect with existing range constraints anyway (which it seems we do in WitgenInference::add_range_constraint), so it's no-op.

[georgwiese]

Suggested change

	let mut values = index
	.iter()
	.filter(|(key, _)| matches_range_constraint(key, &input_range_constraints))
	.map(|(_, value)| {
	let (_, values) = value.get()?;
	Some(values)
	});
	let Some(first) = values.next() else {
	// If the iterator is empty, we have no match, but it can always be answered,
	// so this is successful.
	// Unfortunately, we cannot express the "empty" range constraint.
	return Some(vec![Default::default(); range_constraints.len()]);
	};
	// If an item (including the first) is None, it means the match was not unique.
	let mut new_output_range_constraints = first?
	.iter()
	// min, max, mask
	.map(|v| (*v, *v, v.to_integer()))
	.collect_vec();
	for v in values {
	// If any value is None, the match was not unique.
	let v = v?;
	for ((min, max, mask), v) in new_output_range_constraints.iter_mut().zip_eq(v) {
	*min = (*min).min(*v);
	*max = (*max).max(*v);
	*mask |= v.to_integer();
	}
	}
	let values = index
	.iter()
	.filter(|(key, _)| matches_range_constraint(key, &input_range_constraints))
	.map(|(_, value)| value.get().map(|(_, values)| values))
	.collect_vec();
	if values.is_empty() {
	// If the iterator is empty, we have no match, but it can always be answered,
	// so this is successful.
	// Unfortunately, we cannot express the "empty" range constraint.
	return Some(vec![Default::default(); range_constraints.len()]);
	};
	// If any item is None, it means the match was not unique.
	let values = values.into_iter().collect::<Option<Vec<_>>>()?;
	let new_output_range_constraints = values
	.into_iter()
	// Map each row to a vector of range constraints, represented as (min, max, mask).
	.map(|row| row.iter().map(|v| (*v, *v, v.to_integer())).collect_vec())
	// Reduce by disjunction
	.reduce(|acc, values| {
	acc.into_iter()
	.zip_eq(values)
	.map(|((min1, max1, mask1), (min2, max2, mask2))| {
	(min1.min(min2), max1.max(max2), mask1 | mask2)
	})
	.collect()
	})
	.unwrap();

What do you think of this? Should be equivalent and removes the special handling of the first element.

[georgwiese]

I think it could be further improved by implementing a disjunction method in RangeConstraint and using it directly, instead of working with (T, T, T::Integer)!

[chriseth]

If the range is really big, this allocates a large vector, I wanted to avoid that.

[chriseth]

Disjunction is more complicated because we need to consider all combinations of which range constraint wraps.

[chriseth]

Actually let me try using reduce.

[chriseth]

Ah right, we would need try_reduce, but it's not stable.

[chriseth]

Changed it to avoid advancing the iterator by the first element, but to be honest, I'm not sure it is clearer this way.

[georgwiese]

I think it is clearer, thanks! Just some tiny comments left.

[georgwiese]

I still don't understand whether this is a case that would happen in practice (for example because some branches are actually impossible), or whether hitting this path would mean we can't complete the code generation. If the latter, maybe we can at least log an error?

[chriseth]

I think this can happen if we decide to branch, then derive further concrete values, do a lookup again and then discover that we do not have these values in the lookup table at all.

[github-actions]

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Benchmarks'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite	Current: c13dc10	Previous: 65a2fdd	Ratio
jit-benchmark/sqrt_879882356	18256 ns/iter (± 58)	2620 ns/iter (± 2)	6.97
jit-benchmark/sqrt_1882356	14032 ns/iter (± 44)	2102 ns/iter (± 2)	6.68
jit-benchmark/sqrt_1187956	13774 ns/iter (± 38)	2076 ns/iter (± 2)	6.63
jit-benchmark/sqrt_56	7163 ns/iter (± 33)	1242 ns/iter (± 1)	5.77
jit-benchmark/sort_33	388409 ns/iter (± 1506)	69275 ns/iter (± 73)	5.61
jit-benchmark/sort_100	1521434 ns/iter (± 5676)	257883 ns/iter (± 230)	5.90
jit-benchmark/sort_300	6547032 ns/iter (± 38942)	990021 ns/iter (± 1159)	6.61
jit-benchmark/sort_900	34337552 ns/iter (± 159608)	4214293 ns/iter (± 4534)	8.15
jit-benchmark/sort_2700	222478643 ns/iter (± 1281608)	20424753 ns/iter (± 158091)	10.89

This comment was automatically generated by workflow using github-action-benchmark.