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

Add overload support in lpython #187

Merged
merged 14 commits into from
Mar 9, 2022
Merged

Conversation

Smit-create
Copy link
Collaborator

@Smit-create Smit-create commented Feb 28, 2022

Fixes #141.

@Smit-create Smit-create marked this pull request as draft February 28, 2022 15:10
@certik
Copy link
Contributor

certik commented Feb 28, 2022

I think the symbol table visitor can do all of this, correct? Or do we need to handle this in the body visitor also?

If just symbol table visitor, then I think what you can do is to start the ASR GenericProcedure node, and just keep adding to it. I think. You can see how this is done in LFortran, I can't remember right now. Otherwise using a temporary structure like you have done in this PR also works.

@Smit-create
Copy link
Collaborator Author

ASR GenericProcedure node,

Will have a look how that is done.

@Smit-create
Copy link
Collaborator Author

On the current diff, I get:

from ltypes import overload, i32

@overload
def foo(a: i32, b: i32) -> i32:
    return a*b

@overload
def foo(a: i32) -> i32:
    return a**2

ASR:

(TranslationUnit 
   (SymbolTable 
      1 
      {
         __lpython_overloaded_0__foo: 
            (Function 
               (SymbolTable 
                  2 
                  {
                     _lpython_return_variable: 
                        (Variable 
                           2 
                           _lpython_return_variable 
                           ReturnVar () () 
                           Default 
                           (Integer 4 []) 
                           Source 
                           Public 
                           Required .false.), 
                     a: 
                        (Variable 
                           2 
                           a 
                           In () () 
                           Default 
                           (Integer 4 []) 
                           Source 
                           Public 
                           Required .false.), 
                     b: 
                        (Variable 
                           2 
                           b 
                           In () () 
                           Default 
                           (Integer 4 []) 
                           Source 
                           Public 
                           Required .false.)
                  }) 
               __lpython_overloaded_0__foo [
               (Var 2 a) 
               (Var 2 b)] [
               (= 
                  (Var 2 _lpython_return_variable) 
                  (BinOp 
                     (Var 2 a) 
                     Mul 
                     (Var 2 b) 
                     (Integer 4 []) () ()) ()) 
               (Return)] 
               (Var 2 _lpython_return_variable) 
               Source 
               Public 
               Implementation ()), 
         __lpython_overloaded_1__foo: 
            (Function 
               (SymbolTable 
                  3 
                  {
                     _lpython_return_variable: 
                        (Variable 
                           3 
                           _lpython_return_variable 
                           ReturnVar () () 
                           Default 
                           (Integer 4 []) 
                           Source 
                           Public 
                           Required .false.), 
                     a: 
                        (Variable 
                           3 
                           a 
                           In () () 
                           Default 
                           (Integer 4 []) 
                           Source 
                           Public 
                           Required .false.)
                  }) 
               __lpython_overloaded_1__foo [
               (Var 3 a)] [
               (= 
                  (Var 3 _lpython_return_variable) 
                  (BinOp 
                     (Var 3 a) 
                     Pow (ConstantInteger 2 
                     (Integer 4 [])) 
                     (Integer 4 []) () ()) ()) 
               (Return)] 
               (Var 3 _lpython_return_variable) 
               Source 
               Public 
               Implementation ()), 
         main_program: 
            (Program 
               (SymbolTable 
                  4 
                  {
                  }) 
               main_program [] [])
      }) 
   [])

@Smit-create
Copy link
Collaborator Author

We will now need to add support for overload in Call

@certik
Copy link
Contributor

certik commented Mar 1, 2022

The ASR example you posted is almost ok, but it needs to also add foo: GenericProcedure(__lpython_overloaded_0__foo, __lpython_overloaded_1__foo) into the symbol table.

@Smit-create
Copy link
Collaborator Author

The ASR example you posted is almost ok, but it needs to also add foo: GenericProcedure(__lpython_overloaded_0__foo, __lpython_overloaded_1__foo) into the symbol table.

I thought since we are changing the name of the function/subroutine, it might not be that important to use GenericProcedure.

@certik
Copy link
Contributor

certik commented Mar 2, 2022

What is your idea/design of making this work without a GenericProcedure in ASR? By storing the list of overloaded procedures in overload_defs inside AST to ASR, but not in ASR itself?

How would that work with independent compilation of modules to "mod" files, and then loading them in a new lpython invocation?

The idea that we have followed so far is to keep ASR self-contained, so that it contains all the information that you might need later. It seems in this PR we are not storing all the information for generic subroutines in ASR, but keeping it outside of it.

@Smit-create
Copy link
Collaborator Author

The idea that we have followed so far is to keep ASR self-contained, so that it contains all the information that you might need later. It seems in this PR we are not storing all the information for generic subroutines in ASR, but keeping it outside of it.

Yeah, that makes sense to me now. I'll try it using GenericProcedure.

@Smit-create Smit-create marked this pull request as ready for review March 3, 2022 18:59
@Smit-create Smit-create requested a review from certik March 3, 2022 19:00
@Smit-create
Copy link
Collaborator Author

The PR looks all good to go once the tests passes. Just need to figure out how to get rid of the global variable ast_overload.

@@ -139,6 +139,8 @@ ASR::Module_t* load_module(Allocator &al, SymbolTable *symtab,
return mod2;
}

std::map<int, ASR::symbol_t*> ast_overload;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The last part to do is to pass this via arguments to the constructor of the visitor classes and have it as a local variable in python_ast_to_asr. Then it will not clash when modules are imported recursively.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then it will not clash when modules are imported recursively.

That's fine, but I guess it won't be a big issue since we are storing the memory reference of each function which will indeed be unique.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only if you get lucky. When you load multiple modules, the visitor gets created, then destroyed. I think the nodes are allocated using our linear allocator, so I guess they would still be unique. But then when you compile multiple stuff from the main driver and happen to create a new allocator, then it will possibly collide. So we should not rely on that and have no global state.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about using it as a class variable and clearing it completely when a symbol table visitor is instantiated?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That will erase it when you import other modules recursively, wouldn't it?

Copy link
Collaborator Author

@Smit-create Smit-create Mar 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, yes! One probable approach can be to add an optional arguement in FunctionDef to store the index where it will be present in GenericNode.

@certik
Copy link
Contributor

certik commented Mar 3, 2022

Otherwise I think this looks good.

@certik
Copy link
Contributor

certik commented Mar 6, 2022

Let me know if you need any help with implementing the passing of the ast_overload into the visitor patterns via their constructors.

Copy link
Contributor

@certik certik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved the ast_overload into a local variable, so it should be good to go in now. Thanks @Smit-create !

@certik certik enabled auto-merge March 9, 2022 17:47
@certik certik merged commit 37e2685 into lcompilers:main Mar 9, 2022
@Smit-create Smit-create deleted the overload3 branch March 10, 2022 05:49
@Smit-create
Copy link
Collaborator Author

Thanks for updating the PR and merging!!

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

Successfully merging this pull request may close these issues.

Figure out how to handle generic functions
3 participants