Skip to content
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

Feature request: allow builder::name to take integers as template parameters #80

Closed
RadhikaG opened this issue Sep 6, 2024 · 10 comments
Assignees
Labels
enhancement New feature or request

Comments

@RadhikaG
Copy link
Contributor

RadhikaG commented Sep 6, 2024

Currently builder::name can only take typenames as template parameters.

This prevents us from having types in the generated code that take integers as template params, so we can't generate variables with type, say Eigen::Matrix<double, 5, 1> (a matrix with size 5x1 fixed at compile time).

Could you please add support for integer template type parameters? Thank you!

@AjayBrahmakshatriya
Copy link
Collaborator

AjayBrahmakshatriya commented Sep 6, 2024

Hi @RadhikaG
This is a great suggestion and we have thought about adding this before. However, there is a limitation with C++ that doesn't allow this. Let me explain below and suggest a workaround -

Currently builder::name is defined as -

template <const char *N, typename... Args>
struct name {};

The variable template arg pack unfortunately cannot be of a mixed "kind". It has to either be all types or all values. We decided to go along with the type version since it is the most common use case.

To make this work, you can fallback to the custom types approach. You can start by creating a struct as follows -

auto named_type_from_int(int x) {
    auto t = std::make_shared<block::named_type>();
    t->type_name = std::to_string(x);
    return t;
}

template <typename T, int...sizes> 
struct EigenMatrix {
    static constexpr const char* type_name = "Eigen::Matrix";
    
    static auto get_template_arg_types() {
        std::vector<block::type::Ptr> types = {dyn_var<T>::create_block_type(), named_type_from_int(sizes)...};
        return types;
    }
};

There are a couple of things happening here. I think you have already seen the behavior of type_name member for custom types in BuildIt. Basically any struct with this member present will borrow the name from it. In the same way BuildIt also looks for the get_template_arg_types function if present and uses it to construct the template arguments for the type. It expects the function to return a vector of types. What I have done here is called the usual dyn_var<T>::create_block_type() to get the type node for the first argument and converted the rest of the integer arguments to a type by creating a named_type for them with the named_type_from_int helper function and expanded it.

You can see the example working here - https://buildit.so/tryit/?sample=shared&pid=3c23328edec2f3e1e9bf6b72d0870336

Would this work for you for now?

As for big picture, if you are working on specializing Eigen for specific sizes, do you really need to generate Eigen::Matrix objects? Should you instead not make the internal array member dyn_var? That way the generated code will just have scalar types and their arrays? The last point is just a suggestion, please disregard if it doesn't fit your use case.

I will also at some point add some of the helpers to BuildIt itself to simplify this and add an example.

Thanks

@RadhikaG
Copy link
Contributor Author

RadhikaG commented Sep 7, 2024

Thanks for the workaround @AjayBrahmakshatriya, will try it out and get back to you!

ref big picture: there are two ways we're interested in using Eigen:

  1. Input program: uses linalg exprs of Eigen::Matrix<dyn_var<scalar>> types -> BuildIt output: generates scalar unrolled math. ("soup of scalars" as I like to call it).
  2. Input program: uses linalg exprs of custom matrix types we provide -> BuildIt output: generates linalg exprs of Eigen::Matrix<scalar> types.
    • This usecase is the subject of this GitHub issue. We eventually want to try out using Highway as a backend too.

I had to pause dev on (1) for a little bit, but I will need to get around to it soon... I did however mean to schedule a meeting with you at some point to solicit some compiler design advice!

Thanks once again for the help! We've managed to generate some interesting code using BuildIt for our larger research project already (which I'd be happy to talk more about), and we appreciate the number of examples you've provided!

@AjayBrahmakshatriya AjayBrahmakshatriya self-assigned this Sep 8, 2024
@AjayBrahmakshatriya AjayBrahmakshatriya added the enhancement New feature or request label Sep 8, 2024
@AjayBrahmakshatriya
Copy link
Collaborator

Hi @RadhikaG ,
That sounds wonderful. I am very excited to hear about all the use cases you have so far. Also happy to hop on a call for the compiler advice/discussion. Let's plan that through email.

Also, if this workaround helps with your use case, feel free to close the issue. I don't think otherwise a change is required to BuildIt right now. But I am open to suggestions if you think there is a better interface to expose to users.

Thanks

@RadhikaG
Copy link
Contributor Author

RadhikaG commented Nov 27, 2024

Hi @AjayBrahmakshatriya, took your solution for spin, it works well for us when we know the matrix size at compile-time!

Do you by any chance have a good way to generate an Eigen::Matrix<Scalar, r, c> type, where r and c are calculated in the first stage? Something like this snippet? (this obv does not compile, but it captures my intention, the hope is that it generates a Eigen::Matrix<double, 5, 1> var0)

Maybe this is quite hard because BuildIt can extract types only from template args?

@AjayBrahmakshatriya
Copy link
Collaborator

AjayBrahmakshatriya commented Nov 30, 2024

Hi @RadhikaG glad the solution works for compile time constants. As for first stage values as type parameters that can also be done.
Basically the idea is to set the type of the variable after the type variable has been declared. Here is an example -

https://buildit.so/tryit/?sample=shared&pid=f5c939a7d6830ac609444dabba56df1d

Basically you can create a helper function that can manipulate the type of the variable after it has been declared. As long it is called sometime during the lifetime of the variable it should work. If you accept such variable as function arguments they can be fixed at the beginning of the function. If you are wrapping these variable in another class, you can also do it in the constructor of the wrapper type. It is possible to do it in the constructor for Eigen too, but currently there is no way to forward constructor arguments to Eigen from dyn_var. However, you could also change the specialization for dyn_var<Eigen> and add a new constructor. Let me know if you want an example for that.

This can also be made a member function inside Eigen if that is easier. Just remember to cast this to dyn_var<Eigen<T>>* since dyn_var<Eigen> inherits from Eigen.

@RadhikaG
Copy link
Contributor Author

RadhikaG commented Dec 9, 2024

@AjayBrahmakshatriya thanks for your help! I took a stab at implementing a specialization for dyn_var<EigenMatrix> here.

(this is an approximation of how we're using dyn_vars to emit instructions from another wrapper class Matrix)

This version is pretty darn close to what we want, but it messes up the copy (line 3 and 6 in generated code, should be Eigen::Matrix<double, 3, 4>, I have likely made an error in the dyn_var copy constructor), and I'm unsure how to get the return type of the function right (line 1 in generated code, I want it to be Eigen::Matrix<double, 3, 4>). I have also deferred the initialization of the dyn_var<EigenMatrix> member variable incorrectly.

Do you have any implementation tips for fixing the return type and the copied type?

@AjayBrahmakshatriya
Copy link
Collaborator

AjayBrahmakshatriya commented Dec 9, 2024

Hi @RadhikaG,
I took a quick look and your copy constructor seems to be correct. There was actually an issue with your Matrix constructor where you were calling deferred_init after the regular constructor. So it was resetting the type for the original variable to be just just double. The copy was just copying over this double.
Here is the working version - https://buildit.so/tryit/?sample=shared&pid=85a7a6d6a8fbc17785a4c84fbed56404

If you do need deferred init for any reason, you might have to overload the deferred_init function in your specialization. Just call the original deferred_init and set the type like you would in the constructor.

One thing still remains unclear is how is the original variable getting the appropriate type despite being reset. But that shouldn't be an issue for you. Calling deferred_init on an already initialized variable is undefined behavior in BuildIt any way. I will investigate and post it here if you are curious as to exactly what was happening.

Turns out the original variable in the generated code was getting the right type because when you call deferred_init a new block::var is created with a fresh new type. However the original block::decl_stmt is still pointing to the old variable.

Let me know if this fixed version works for you.

Thanks

@RadhikaG
Copy link
Contributor Author

RadhikaG commented Dec 9, 2024

Thanks for your quick reply, your version fixed the copy issue! Do you have a good fix for the return type problem?

The return type of bar is currently Eigen::Matrix<double>: I'd like it to be Eigen::Matrix<double, s1, s2>, but I don't know how to implement this.

@AjayBrahmakshatriya
Copy link
Collaborator

@RadhikaG , Right! Missed that part of your question.

So that type isn't connected to any variable and is directly set here - https://github.com/BuildIt-lang/buildit/blob/master/include%2Fbuilder%2Fsignature_extract.h#L84

The easiest way to get the appropriate type could be to set it after the function has been extracted? The func_decl returned has a member called return_type.

If it truly depends on the function body you could return the size through a ref argument (non-dyn_var) and then the calling code can patch in the type?

I understand it's not the cleanest solution, but it should work. If you want a super cleam solution, you might have to extend the builder_context to derive the return type based on the return value.

@AjayBrahmakshatriya
Copy link
Collaborator

@RadhikaG closing this for now. But feel free to open it in case something comes up.

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants