-
Notifications
You must be signed in to change notification settings - Fork 22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
struct Dav1dLoopRestorationDSPContext
: Deduplicate via type-erased fn
pointers
#332
struct Dav1dLoopRestorationDSPContext
: Deduplicate via type-erased fn
pointers
#332
Conversation
This PR seems to perform on par with
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 I really like how this PR doesn't need a lot of macros; I find this style more readable.
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks pretty great to me. I left just a few small comments to fix before merging (can we squash to keep all commits to main
CI-passing?), as well as a few comments about further improvements for later PRs and once bitdepth genericization is done.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
fn
per macro call, so go-to-definition is easier to use - keeps the
fn $name
together so that it's more easily greppable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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, maybefn 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.
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.
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
, wherepixel
has been defined asc_void
. For the 8bpc versions, an extrabitdepth_max: c_int
argument 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..