vello_cpu: Avoid continuously splatting positions#1728
Conversation
|
I have resolved the issue mentioned above by just increasing the (already-existing!) threshold for now, I hope this is okay. |
2790745 to
dc6f551
Compare
LaurenzV
changed the title
dc6f551 to
4c1c556
Compare
| pub(crate) fn advance(&mut self) { | ||
| self.x_pos += self.x_step; | ||
| self.y_pos += self.y_step; | ||
| } |
There was a problem hiding this comment.
Since we're now accumulating the position in f32 (pos += step each chunk), the rounding error compounds over the span — which I think is what pushed gradient_linear_with_y_repeat from diff_pixels = 2 to 80. Would deriving each position from a fixed base with an FMA instead of mutating it avoid the drift while staying register-resident? Something like:
// keep x_base/x_step splatted once, plus a lane-uniform chunk counter `i`
fn current(&self) -> V {
// single rounding, error doesn't accumulate
self.i.mul_add(self.x_step, self.x_base)
}
fn advance(&mut self) {
// single rounding, error doesn't accumulate
self.i += 1.0;
}That should keep the error bounded regardless of span width (so the threshold could maybe go back to ~2?). Might be worth a quick check.
There was a problem hiding this comment.
I tried this, and overall doesn't seem to impact performance, so I think I like it! It reduces the number of diff pixels (from about ~50 to ~12), but still doesn't completetly eliminate them. I feel like this is acceptable?
There was a problem hiding this comment.
I think it's much better now. Would it still be possible to investigate the reason for the discrepancies? It could be that we're applying an additional compensation offset in hybrid, so we might be able to adjust or remove it.
| } | ||
| } | ||
|
|
||
| impl<S: Simd> PositionIterator<S> for PaintPositions<S, f32x8<S>> { |
There was a problem hiding this comment.
Small thing: PositionIterator methods here forward to the inherent methods of the same name via Self::advance(self), which only works because inherent methods win resolution over trait ones. Could we fold the logic into a single blanket impl over V and drop the inherent new/advance, so there's only one definition? e.g.:
impl<S, V> PositionIterator<S> for PaintPositions<S, V>
where
S: Simd,
V: PosExt<S> + SimdBase<S, Element = f32> + core::ops::AddAssign,
{
fn advance(&mut self) {
self.x_pos += self.x_step;
self.y_pos += self.y_step;
}
// ...
}The f32x4 image painters would then get the trait for free too. Does that work, or was the split intentional for some reason I'm missing?
There was a problem hiding this comment.
Yup makes sense, thanks!
| // color is sampled. This threshold was previously only 2, but had to be increased due to | ||
| // a change in vello_cpu (see https://github.com/linebender/vello/pull/1728) which changed | ||
| // the accumulation behavior and leads to higher discrepancies compared to Vello Hybrid. | ||
| #[vello_test(diff_pixels = 80)] |
There was a problem hiding this comment.
One of the ideas I mentioned above could help us avoid using such a high threshold, but I think we should try to minimize these kinds of discrepancies with hybrid/cpu as much as possible. Would it be possible to investigate the accumulation behaviour further and see if we can fix it?
8b6c2b0 to
fdb4016
Compare
~~Based on top of linebender#1728.~~ A painter is supposed to be initialized and then only used once, since each time we change the location we need to reinitialize it. This PR changes the signature so this intention is properly encoded in the `paint_*` methods. This PR was done with assistance of GPT 5.6-Sol.
2 participants
For our image and gradient painters, the overall flow right now is that we iterate over a chunk of pixels, keep track of the current x/y position using a scalar f64, at the start of each pixel chunk we calculate our current position and store it in a SIMD vector. At the end, we advance the scalar position value just to redo the same position splatting in the next iteration.
This is wasteful, because
splat_posis quite expensive. Apart from that, it also doesn't allow the compiler to retain the positions in the registers between iterations, even though this is possible in most cases. Therefore, this PR changes it such that we calculate the start position only once in the beginning, and then advance it using normal SIMD arithmetics at the end of each iteration. The benchmark improvements are rather astonishing, see below!However, I suppose there is no free lunch. This approach does have the problem that we are doing the accumulation using f32 instead of f64, which has less precision, but since we our pixmap is limited to
u16::MAX, this is still more than enough. Nearly all tests just had a max channel delta of 1 and only very few changed pixels. However, there is one test which now has a very large diff due to slight differences for some pixels and can therefore not be directly compared against Vello hybrid anymore. :(Open to suggestions for how to best handle this.EDIT: See comment further velow.On NEON:
On AVX2:
This PR was done with assistance of GPT 5.5, high.