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.

Sign up for GitHub

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

Jump to bottom

Georgemitenkov/llvm indexed names #478

Merged
merged 7 commits into from
Dec 30, 2020
Merged

Georgemitenkov/llvm indexed names #478

merged 7 commits into from
Dec 30, 2020

Conversation

georgemitenkov
Copy link
Collaborator

@georgemitenkov georgemitenkov commented Dec 30, 2020 •
edited
Loading

This PR addresses IndexedNames, adding the following:

  • Initialising arrays in LOCAL blocks is now supported.
  • Assigning to array elements is supported.
  • Using array elements in expressions is possible now.

Note that this handles 1D arrays (same as NMODL)

Also, codegen for DEFINE node has been implemented: macros are currently stored in a dedicated map.

fixes #467

@bbpbuildbot
Copy link
Collaborator

Can one of the admins verify this patch?

@georgemitenkov georgemitenkov linked an issue Dec 30, 2020 that may be closed by this pull request
Support indexed values #467
Closed
pramodk
pramodk reviewed Dec 30, 2020
View reviewed changes
Copy link
Contributor

@pramodk pramodk left a comment

Choose a reason for hiding this comment

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

lgtm except the comment about using macro values from global symtab.

src/codegen/llvm/codegen_llvm_visitor.cpp Outdated
}

unsigned CodegenLLVMVisitor::get_array_index_or_length(const ast::IndexedName& indexed_name) {
auto integer = dynamic_cast<ast::Integer*>(indexed_name.get_length().get());
Copy link
Contributor

@pramodk pramodk Dec 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

just a stylistic comment : there is also dynamic_pointer_cast with which you don't need .get()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for letting me know about this!

src/codegen/llvm/codegen_llvm_visitor.cpp Outdated
Comment on lines 60 to 63
if (!integer->get_macro())
return integer->get_value();
return macros[integer->get_macro()->get_node_name()];
}
Copy link
Contributor

Choose a reason for hiding this comment

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

In the global symbol table we store macros and their values. Can that be used?

[NMODL] [info] :: Printing symbol table

--------------------------------------------------------------------------------------------------------
|                   NMODL_GLOBAL [Program IN None] POSITION : UNKNOWN SCOPE : GLOBAL                   |
--------------------------------------------------------------------------------------------------------
|     NAME     |     PROPERTIES     |  STATUS   |  LOCATION   |    VALUE    |  # READS   |  # WRITES   |
--------------------------------------------------------------------------------------------------------
| t            | extern_neuron_var  |           |    EXTERNAL |             |     1      |      0      |
| v            | extern_neuron_var  |           |    EXTERNAL |             |     1      |      0      |
| N            | define             |           |     UNKNOWN |  100.000000 |     0      |      0      |

src/codegen/llvm/codegen_llvm_visitor.cpp Outdated
@@ -165,6 +210,10 @@ void CodegenLLVMVisitor::visit_boolean(const ast::Boolean& node) {
values.push_back(constant);
}

void CodegenLLVMVisitor::visit_define(const ast::Define& node) {
macros[node.get_node_name()] = node.get_value()->get_value();
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question as before : can the symtab be used for this purpose?

src/codegen/llvm/codegen_llvm_visitor.hpp Outdated
* \param index element index
* \return GEP instruction value
*/
llvm::Value* create_GEP(const std::string& name, unsigned index);
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick : stylistically, in nmodl we would write as create_gep (but it's true that in LLVM method has GEP)

@georgemitenkov georgemitenkov force-pushed the georgemitenkov/llvm-indexed-names branch from 2f03bd3 to a6cd161 Compare December 30, 2020 19:27
@georgemitenkov
Copy link
Collaborator Author

@pramodk I used a symbol table instead! However, while testing, I noticed that in C visitor, the macro doesn't seem to be expanded?

DEFINE N 2

PROCEDURE foo() {
        LOCAL x[N]
}

gives

inline int foo_(int id, int pnodecount, _Instance* inst, double* data, const Datum* indexes, ThreadDatum* thread, NrnThread* nt, double v) {
        int ret_foo = 0;
        double x[N];
        return ret_foo;
    }

and there is no definition of N as well.

@pramodk
Copy link
Contributor

pramodk commented Dec 30, 2020

Oops..that's a bug! Didn't notice that earlier as local array is not commonly used. 😒

Could you create an issue with above code snippet? I will then take care of this in C codegen.

@georgemitenkov
Copy link
Collaborator Author

Created an issue :) It seems like only occurring when used for array sizes.

pramodk
pramodk approved these changes Dec 30, 2020
View reviewed changes
Copy link
Contributor

@pramodk pramodk left a comment

Choose a reason for hiding this comment

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

LGTM

@georgemitenkov georgemitenkov merged commit 5435cbe into llvm Dec 30, 2020
@georgemitenkov georgemitenkov deleted the georgemitenkov/llvm-indexed-names branch December 30, 2020 21:49
pramodk pushed a commit that referenced this pull request Feb 23, 2021
@georgemitenkov @pramodk
Support for IndexedName codegen (#478)
cd6cd37 
LLVM code generation for `IndexedName`s.

- Added code generation for initialising arrays in LOCAL blocks (with both integer constants and macros).
- Added support for indexing arrays.

fixes #467
pramodk pushed a commit that referenced this pull request May 8, 2021
@georgemitenkov @pramodk
Support for IndexedName codegen (#478)
7884de8 
LLVM code generation for `IndexedName`s.

- Added code generation for initialising arrays in LOCAL blocks (with both integer constants and macros).
- Added support for indexing arrays.

fixes #467
pramodk pushed a commit that referenced this pull request Mar 8, 2022
@georgemitenkov @pramodk
Support for IndexedName codegen (#478)
d0fe34d 
LLVM code generation for `IndexedName`s.

- Added code generation for initialising arrays in LOCAL blocks (with both integer constants and macros).
- Added support for indexing arrays.

fixes #467
iomaganaris pushed a commit that referenced this pull request May 10, 2022
@georgemitenkov @iomaganaris
Support for IndexedName codegen (#478)
d07ad0e 
LLVM code generation for `IndexedName`s.

- Added code generation for initialising arrays in LOCAL blocks (with both integer constants and macros).
- Added support for indexing arrays.

fixes #467
iomaganaris pushed a commit that referenced this pull request May 12, 2022
@georgemitenkov @iomaganaris
Support for IndexedName codegen (#478)
438dca8 
LLVM code generation for `IndexedName`s.

- Added code generation for initialising arrays in LOCAL blocks (with both integer constants and macros).
- Added support for indexing arrays.

fixes #467
iomaganaris pushed a commit that referenced this pull request Sep 15, 2022
@georgemitenkov @iomaganaris
Support for IndexedName codegen (#478)
470d54a 
LLVM code generation for `IndexedName`s.

- Added code generation for initialising arrays in LOCAL blocks (with both integer constants and macros).
- Added support for indexing arrays.

fixes #467
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pramodk pramodk pramodk approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support indexed values
3 participants
@georgemitenkov @bbpbuildbot @pramodk