core: reduce code duplication in codegen/builtin_fns #422

Merged
sb10q merged 7 commits from refactor-builtin-fns into master 2024-06-20 12:48:45 +08:00
Collaborator

Used macros to generate some unary math functions.

Used macros to generate some unary math functions.
lyken added 2 commits 2024-06-18 17:17:35 +08:00
lyken added 1 commit 2024-06-19 09:48:09 +08:00
sb10q requested review from derppening 2024-06-19 15:15:15 +08:00
lyken added 1 commit 2024-06-19 16:08:17 +08:00
derppening requested changes 2024-06-19 18:03:32 +08:00
@ -1048,0 +1053,4 @@
/// Return a constant [`Type`] here if the return type does not depend on the input type.
/// * `on_elem`: The function to be called when the input argument is not an ndarray. Returns [`Option::None`]
/// if the element type & value are faulty and should panic with [`unsupported_type`].
fn helper_call_numpy<'ctx, OnElemFn, RetElemFn, G: CodeGenerator + ?Sized>(
Collaborator
  • Move the bounds for G into the where clause.
  • call_numpy makes it seem like you can call any numpy function. Perhaps something like call_numpy_unaryfunc?
- Move the bounds for `G` into the `where` clause. - `call_numpy` makes it seem like you can call any numpy function. Perhaps something like `call_numpy_unaryfunc`?
derppening marked this conversation as resolved
@ -1048,0 +1059,4 @@
(arg_ty, arg_val): (Type, BasicValueEnum<'ctx>),
fn_name: &str,
get_ret_elem_type: &RetElemFn,
on_elem: &OnElemFn,
Collaborator

on_scalar

`on_scalar`
derppening marked this conversation as resolved
@ -1070,3 +1113,1 @@
.any(|ty| ctx.unifier.unioned(n_ty, *ty)));
if [ctx.primitives.int32, ctx.primitives.int64]
let fn_name: &str = "abs";
Collaborator

Is this change from const to let necessary?

Is this change from `const` to `let` necessary?
derppening marked this conversation as resolved
@ -1079,0 +1156,4 @@
)
}
/// Helper macro. Used so we don't have to keep typing out the type signature of the function.
Collaborator

Documentation comments are to explain what this macro does.

/// Macro for generating unary functions accepting  `ndarray`-compatible values.
///
/// The only form of the macro accepts an identifier for the generated function, the name of the 
/// corresponding Python function, a closure of type `Fn(&mut CodeGenContext<'ctx, '_>, Type) -> 
/// Type` which obtains the elementwise return type, and a closure of type `Fn(&mut G, 
/// &mut CodeGenContext<'ctx, '_>, Type, BasicValueEnum<'ctx>) -> Option<BasicValueEnum<'ctx>>` 
/// which performs the operation on a single element.

Same for the macros below.

Documentation comments are to explain what this macro does. ```rs /// Macro for generating unary functions accepting `ndarray`-compatible values. /// /// The only form of the macro accepts an identifier for the generated function, the name of the /// corresponding Python function, a closure of type `Fn(&mut CodeGenContext<'ctx, '_>, Type) -> /// Type` which obtains the elementwise return type, and a closure of type `Fn(&mut G, /// &mut CodeGenContext<'ctx, '_>, Type, BasicValueEnum<'ctx>) -> Option<BasicValueEnum<'ctx>>` /// which performs the operation on a single element. ``` Same for the macros below.
derppening marked this conversation as resolved
@ -1079,0 +1157,4 @@
}
/// Helper macro. Used so we don't have to keep typing out the type signature of the function.
macro_rules! call_numpy {
Collaborator

I don't think it's appropriate to name the macro call_numpy, as it gives the impression that the macro actually invokes the function rather than generates a function which can in turn be used.

Same for the macros below.

I don't think it's appropriate to name the macro `call_numpy`, as it gives the impression that the macro actually invokes the function rather than generates a function which can in turn be used. Same for the macros below.
derppening marked this conversation as resolved
lyken added 1 commit 2024-06-20 10:31:00 +08:00
Author
Collaborator

Revised.

About the macro comments, I have taken the liberty to make changes to the suggested update.

Revised. About the macro comments, I have taken the liberty to make changes to the suggested update.
derppening approved these changes 2024-06-20 11:40:38 +08:00
derppening left a comment
Collaborator

Apart from the nits, the rest looks good to me.

Apart from the nits, the rest looks good to me.
@ -1049,3 +1109,3 @@
generator: &mut G,
ctx: &mut CodeGenContext<'ctx, '_>,
n: (Type, BasicValueEnum<'ctx>),
arg: (Type, BasicValueEnum<'ctx>),
Collaborator

Nit: Please keep this parameter as n. I don't think this change is necessary.

Nit: Please keep this parameter as `n`. I don't think this change is necessary.
@ -1079,0 +1192,4 @@
/// * `$fn_name:literal`: To be passed to the `fn_name` parameter of [`helper_call_numpy_unary_elementwise`].
/// * `$on_scalar:expr`: The closure (see below for its type) that acts on float scalar values and returns
/// the boolean results of LLVM type `i1`. The returned `i1` value will be converted into an `i8`.
/// ```rust
Collaborator

Nit: Please add a newline between this and the last line.

Same for the macro below.

Nit: Please add a newline between this and the last line. Same for the macro below.
lyken added 2 commits 2024-06-20 11:47:22 +08:00
Author
Collaborator

Revised.

Revised.
Collaborator

Please squash the minor documentation/naming changes into their respective commits.

Please squash the minor documentation/naming changes into their respective commits.
Author
Collaborator

Could we instead make a squash commit? Because I think the history of this branch does not matter too much and might pollute the master branch. The branch also contains commits that revert parts of previous commits.

Could we instead make a squash commit? Because I think the history of this branch does not matter too much and might pollute the master branch. The branch also contains commits that revert parts of previous commits.
Collaborator

Sounds good to me.

Sounds good to me.
derppening requested review from sb10q 2024-06-20 12:39:22 +08:00
sb10q merged commit e36af3b0a3 into master 2024-06-20 12:48:45 +08:00
sb10q deleted branch refactor-builtin-fns 2024-06-20 12:48:45 +08:00
Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: M-Labs/nac3#422
No description provided.