struct Dav1dLoopRestorationDSPContext: Deduplicate via type-erased fn pointers
#332
Conversation
|
This PR seems to perform on par with
|
| @@ -715,10 +715,10 @@ unsafe extern "C" fn lr_stripe( | |||
| & LR_HAVE_BOTTOM as libc::c_int as libc::c_uint, | |||
| ); | |||
| lr_fn.expect("non-null function pointer")( | |||
| p, | |||
| p.cast(), | |||
There was a problem hiding this comment.
We'll want to put this behind a typed interface eventually. I think when we genericize over bitdepth is a good time to do that.
src/looprestoration.rs
Outdated
| // TODO(randomPoison): Temporarily pub until init fns are deduplicated. | ||
| #[cfg(all( | ||
| feature = "bitdepth_8", | ||
| feature = "asm", | ||
| any(target_arch = "x86", target_arch = "x86_64"), | ||
| ))] | ||
| extern "C" { | ||
| pub(crate) fn dav1d_wiener_filter7_8bpc_sse2( | ||
| dst: *mut pixel, |
There was a problem hiding this comment.
Do we want to use a macro to generate these? Is that worth it? It'd look much cleaner and the C version uses macros for this, too. We can do this in a follow-up PR, though.
There was a problem hiding this comment.
Do we want to use a macro to generate these?
IMO it is best to avoid compactifying code using macros. If often interferes with stuff like go-to-definition, grepping for a symbol, and debugging (which is why we need aids like -Zunpretty=expanded).
There was a problem hiding this comment.
Oh, before I saw these comments I went and rewrote this to use a macro 😅
@thedataking I understand your concerns with using macros generally, but in this case I think it's worth it. In addition to making our code more compact, using a macro also has the advantage that it makes it much easier to see which functions all share the same signature, which I think is a big win for readability of the code.
kkysen
changed the title
struct Dav1dLoopRestorationDSPContext: Deduplicate via type-erased fn pointers
src/looprestoration.rs
Outdated
| macro_rules! extern_fn { | ||
| ( $( $name:ident, )* ) => { | ||
| extern "C" { | ||
| $( | ||
| // TODO(randomPoison): Temporarily pub until init fns are deduplicated. | ||
| pub(crate) fn $name( | ||
| dst: *mut pixel, | ||
| dst_stride: ptrdiff_t, | ||
| left: const_left_pixel_row, | ||
| lpf: *const pixel, | ||
| w: libc::c_int, | ||
| h: libc::c_int, | ||
| params: *const LooprestorationParams, | ||
| edges: LrEdgeFlags, | ||
| bitdepth_max: libc::c_int, | ||
| ); | ||
| )* | ||
| } | ||
| }; | ||
| } | ||
|
|
||
| #[cfg(all( | ||
| feature = "bitdepth_8", | ||
| feature = "asm", | ||
| any(target_arch = "x86", target_arch = "x86_64"), | ||
| ))] | ||
| extern_fn! { | ||
| dav1d_wiener_filter7_8bpc_sse2, | ||
| dav1d_wiener_filter5_8bpc_sse2, |
There was a problem hiding this comment.
To help with @thedataking's concerns for using a macro for this (#332 (comment)), how about we make the macro something like decl_looprestoration_fn!(fn dav1d_wiener_filter7_8bpc_sse2);? This way it's
- closer to the C version
- declares one
fnper macro call, so go-to-definition is easier to use - keeps the
fn $nametogether so that it's more easily greppable
There was a problem hiding this comment.
I don't think there's a big advantage to only declaring one fn per macro invocation, and I think it mostly increases visual noise. However, I do think doing fn $name is a good idea in order to make it easier to find the fn declarations by grepping 👍
There was a problem hiding this comment.
One-per-invocation does have some important advantages, though. When the macro invocation has an error, it's very difficult to tell from which part of it the error comes from (e.x. https://github.com/immunant/c2rust/blob/0021ec979663b96b265ebf4995b601220bfbac32/c2rust-analyze/src/known_fn.rs#L570-L629). If we have separate macro invocations, it's super clear which invocation causes the error.
Similarly, with go-to-definition, it generally will work much better if there are separate invocations, though I have noticed in some cases that rust-analyzer is smart enough to figure it out still (e.x. https://github.com/immunant/c2rust/blob/0021ec979663b96b265ebf4995b601220bfbac32/c2rust-analyze/tests/filecheck.rs#L33-L65).
There was a problem hiding this comment.
Appreciate the effort to address my concerns. That said, what does the macro buy you? Code compactness and not much else. Code compactness takes a backseat to readability, debuggability, greppability, etc. IMHO.
I often grep for fn dav1d_wiener_filter7_8bpc_sse2 or some variation thereof, maybe fn dav1d_wiener_filter7_8bpc if I only want to see function definitions. Please (please), consider using macros sparingly. C uses macros and typedefs extensively as a way to paper over portability issues. Rust often provides portable types and APIs so the reasonable expectation is for Rust code to use macros less.
A good use of macros is when you have a "can't see the forrest for all the trees" situation where you can't see the overall pattern of the code for the sheer repetitiveness. I think we had situations like that in decode.rs where using macros helped. I just don't think we have the same situation here.
Thanks for hearing out my concerns. Now that I've voiced my concerns, I'll extricate myself from this discussion.
There was a problem hiding this comment.
Yeah, it seems like rust-analyzer is able to figure it out in this case, likely because the macro is so simple.
Another reason to keep the current version is that it makes it easier to group fns by their #[cfg] attribute. Doing one macro invocation per function means that the #[cfg] attributes would need to be duplicated a lot, which hurts readability.
I think what I have currently is cleaner than the proposed alternative, but if you feel strongly that we should go with a different approach then I can make the change.
There was a problem hiding this comment.
Another reason to keep the current version is that it makes it easier to group fns by their
#[cfg]attribute. Doing one macro invocation per function means that the#[cfg]attributes would need to be duplicated a lot, which hurts readability.
I hadn't thought of that. That definitely helps a lot then. But the macro invocation per function could also be done by putting the extern "C" block outside of the macro invocations, where we could add the #[cfg]s.
There was a problem hiding this comment.
if you feel strongly that we should go with a different approach then I can make the change
I'm fine with what you have, keeping my LGTM. I wasn't factoring in that the macro helped you group fns by cfg. Also good to know that grepping and navigation is expected to work 👍
There was a problem hiding this comment.
Appreciate the effort to address my concerns. That said, what does the macro buy you? Code compactness and not much else. Code compactness takes a backseat to readability, debuggability, greppability, etc. IMHO.
I often grep for
fn dav1d_wiener_filter7_8bpc_sse2or some variation thereof, maybefn dav1d_wiener_filter7_8bpcif I only want to see function definitions. Please (please), consider using macros sparingly. C uses macros and typedefs extensively as a way to paper over portability issues. Rust often provides portable types and APIs so the reasonable expectation is for Rust code to use macros less.A good use of macros is when you have a "can't see the forrest for all the trees" situation where you can't see the overall pattern of the code for the sheer repetitiveness. I think we had situations like that in
decode.rswhere using macros helped. I just don't think we have the same situation here.
To me, the macro here is more readable because it lets you clearly see that every function has the same signature. That's obscured when you see a wall of code. There's not that much use in reading each function individually as they're all just variations of the same function. The macros we had in the devirtualization PRs were a lot more complex, but I think this one is very simple and makes it easier for me to read, and I think for @randomPoison, too.
randomPoison
deleted the
randompoison/looprestorationfilterfn-type-erased
branch
Alternative to #323 that approaches the problem of deduplicating bitdepth-dependent fn pointers by type-erasing the bitdepth-dependent parts. To accomplish this, the relevant
extern "C"declarations were moved intolooprestoration.rs, wherepixelhas been defined asc_void. For the 8bpc versions, an extrabitdepth_max: c_intargument has been added so that the 8bpc and 16bpc fns have the same signature, with the understanding that this extra argument will simply be ignored by the called asm routine. For fn pointers that are defined in Rust code, type-erased wrappers have been added..