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

ASR pass for unique symbols #2149

Merged
merged 9 commits into from
Jul 15, 2023
Merged

ASR pass for unique symbols #2149

merged 9 commits into from
Jul 15, 2023

Conversation

Smit-create
Copy link
Collaborator

@Smit-create
Copy link
Collaborator Author

@certik This PR now passes all the integration tests with LLVM and C backend with every possible name-mangling. It just fails for one test and that has something to do with nested structs and External Symbol. Can you please test this PR against your use cases?

@Smit-create
Copy link
Collaborator Author

Both the test cases in #2129 as well as test_math_02.py work fine with the C backend on this PR.

@certik
Copy link
Contributor

certik commented Jul 11, 2023

I would structure this ASR pass like this:

  • First create a hashmap of unique symbol hash -> (new name) of all symbols that you want to rename and choose how to rename them. Make it configurable how this choice will be made, at the very least it looks like we will need the following options:
    • module name mangling on/off
    • only mangle global symbols, but not symbols in any modules (so that you can call a function from another module with the same name, using an "interface")
    • only global symbols and symbols in intrinsic modules
    • every symbol in all modules
  • Then in the second pass go over and change the name, by changing the symbol, all dependencies to it and all external symbols that mention it. If there are other references elsewhere in ASR, those have to be changed as well.

We can then keep extending the logic in the first pass as needed, and it could be that there is no single way forward, and users will select the behavior on the command line that they need for their application / build system.

@certik
Copy link
Contributor

certik commented Jul 14, 2023

Register the pass in https://github.com/lcompilers/lpython/blob/main/src/libasr/gen_pass.py and run the script to generate the .h file. Keep the generated file committed.

src/bin/lpython.cpp Outdated Show resolved Hide resolved
std::map<std::string, ASR::symbol_t*> current_scope;

UniqueSymbolVisitor(Allocator& al_,
std::map<ASR::symbol_t*, std::string> sn) : al(al_), sym_to_new_name(sn){}
Copy link
Contributor

Choose a reason for hiding this comment

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

a hashtable might be faster here.

@Smit-create Smit-create marked this pull request as ready for review July 15, 2023 05:35
@@ -1616,7 +1635,7 @@ int main(int argc, char *argv[])
app.require_subcommand(0, 1);
CLI11_PARSE(app, argc, argv);

lcompilers_unique_ID = LCompilers::get_unique_ID();
lcompilers_unique_ID = separate_compilation ? LCompilers::get_unique_ID(): "";
Copy link
Contributor

Choose a reason for hiding this comment

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

I would not do that here, but rather have lcompilers_unique_ID always have a unique ID. But have this logic of turning it on and off in asr_scopes. If asr_scopes doesn't have access to separate_compilation then let's create another variable lcompilers_unique_ID_separate_compilation and have that variable on and off as needed.

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 think that this is fine. Let's merge it and we can iterate on this as needed and fix any bugs as we encounter them.

@certik certik merged commit 0b4d13a into lcompilers:main Jul 15, 2023
@Smit-create Smit-create deleted the i-2142-pr branch July 15, 2023 17:33
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.

2 participants