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

Indexed node with expression contains Name and not VarName #545

Open
georgemitenkov opened this issue Mar 8, 2021 · 4 comments
Open

Indexed node with expression contains Name and not VarName #545

georgemitenkov opened this issue Mar 8, 2021 · 4 comments
Labels
ast AST related issues bug Something isn't working good first issue Good for newcomers

Comments

@georgemitenkov
Copy link
Collaborator

While looking at IndexedName node, I noticed that the expression that is used as index created differently from a regular expression. Consider the following program:

PROCEDURE foo() {
    LOCAL x[4], i, j
    i = 0
    j = 1 + i
    x[2 + i] = 3
}

Comparing the AST generated for j = 1 + i and x[2 + i]:

{
                  "BinaryExpression": [
                    {
                      "VarName": [
                        {
                          "Name": [
                            {
                              "String": [
                                {
                                  "name": "j"
                                }
                              ]
                            }
                          ]
                        }
                      ]
                    },
                    {
                      "BinaryOperator": [
                        {
                          "name": "="
                        }
                      ]
                    },
                    {
                      "WrappedExpression": [
                        {
                          "BinaryExpression": [
                            {
                              "Double": [
                                {
                                  "name": "1"
                                }
                              ]
                            },
                            {
                              "BinaryOperator": [
                                {
                                  "name": "+"
                                }
                              ]
                            },
                            {
                              "VarName": [
                                {
                                  "Name": [
                                    {
                                      "String": [
                                        {
                                          "name": "i"
                                        }
                                      ]
                                    }
                                  ]
                                }
                              ]
                            }
                          ]
                        }
                      ]
                    }
                  ]
                }

and

{
                          "IndexedName": [
                            {
                              "Name": [
                                {
                                  "String": [
                                    {
                                      "name": "x"
                                    }
                                  ]
                                }
                              ]
                            },
                            {
                              "WrappedExpression": [
                                {
                                  "BinaryExpression": [
                                    {
                                      "Integer": [
                                        {
                                          "name": "2"
                                        }
                                      ]
                                    },
                                    {
                                      "BinaryOperator": [
                                        {
                                          "name": "+"
                                        }
                                      ]
                                    },
                                    {
                                      "Name": [
                                        {
                                          "String": [
                                            {
                                              "name": "i"
                                            }
                                          ]
                                        }
                                      ]
                                    }
                                  ]
                                }
                              ]
                            }
                          ]
                        }

Here, in indexed case, the binary operator has a Name and not VarName node. this creates an ambiguity for code generation.

@georgemitenkov georgemitenkov added bug Something isn't working ast AST related issues labels Mar 8, 2021
@georgemitenkov georgemitenkov changed the title Indexed node with expression creation contains Name and not VarName Indexed node with expression contains Name and not VarName Mar 8, 2021
@iomaganaris
Copy link
Contributor

Maybe we should fix that in the parser level? @pramodk what do you think?

georgemitenkov added a commit that referenced this issue Mar 12, 2021
Improved index code generation within the LLVM pipeline.
The following issues were addressed:

Array indices are i64 per LLVM's addressing convention.
This means that if the value is not a constant, an additional
sext instruction must be created.

Bounds check is removed since it requires a certain analysis
on the index value. This can be addressed in a separate PR.

`IndexedName` code generation is separated into 2 functions
The first, `get_array_length()` is responsible for array initialisation,
the second, `get_array_index()`, for indexing. In latter case, we
support the following cases:
```
...
// Indexing with an integer constant
k[0] = ...

// Indexing with an integer expression
k[10 - 10] 

// Indexing with a `Name` AST node that is an integer
// (in our case a FOR loop induction variable or a variable
// with `CodegenVarType` == `Integer`
k[id] = ...      
k[ena_id] = ...
```
Note that the case:
```
// id := loop integer induction variable
k[id + 1] = ... 
```
is not supported for 2 reasons:

On the AST level, as per #545 the expression would
contain a Name and not VarName node that fails the
code generation.

The case only arises in the kernel functions like state_update,
where indexing is "artificially" created with indexing by a Name
only.

fixes #541
@pramodk
Copy link
Contributor

pramodk commented Mar 13, 2021

I think this may not be only place where Name and VarName are used interchangeably. We can take a look next week.

@georgemitenkov georgemitenkov added the good first issue Good for newcomers label Apr 15, 2021
pramodk pushed a commit that referenced this issue May 8, 2021
Improved index code generation within the LLVM pipeline.
The following issues were addressed:

Array indices are i64 per LLVM's addressing convention.
This means that if the value is not a constant, an additional
sext instruction must be created.

Bounds check is removed since it requires a certain analysis
on the index value. This can be addressed in a separate PR.

`IndexedName` code generation is separated into 2 functions
The first, `get_array_length()` is responsible for array initialisation,
the second, `get_array_index()`, for indexing. In latter case, we
support the following cases:
```
...
// Indexing with an integer constant
k[0] = ...

// Indexing with an integer expression
k[10 - 10] 

// Indexing with a `Name` AST node that is an integer
// (in our case a FOR loop induction variable or a variable
// with `CodegenVarType` == `Integer`
k[id] = ...      
k[ena_id] = ...
```
Note that the case:
```
// id := loop integer induction variable
k[id + 1] = ... 
```
is not supported for 2 reasons:

On the AST level, as per #545 the expression would
contain a Name and not VarName node that fails the
code generation.

The case only arises in the kernel functions like state_update,
where indexing is "artificially" created with indexing by a Name
only.

fixes #541
pramodk pushed a commit that referenced this issue Mar 8, 2022
Improved index code generation within the LLVM pipeline.
The following issues were addressed:

Array indices are i64 per LLVM's addressing convention.
This means that if the value is not a constant, an additional
sext instruction must be created.

Bounds check is removed since it requires a certain analysis
on the index value. This can be addressed in a separate PR.

`IndexedName` code generation is separated into 2 functions
The first, `get_array_length()` is responsible for array initialisation,
the second, `get_array_index()`, for indexing. In latter case, we
support the following cases:
```
...
// Indexing with an integer constant
k[0] = ...

// Indexing with an integer expression
k[10 - 10] 

// Indexing with a `Name` AST node that is an integer
// (in our case a FOR loop induction variable or a variable
// with `CodegenVarType` == `Integer`
k[id] = ...      
k[ena_id] = ...
```
Note that the case:
```
// id := loop integer induction variable
k[id + 1] = ... 
```
is not supported for 2 reasons:

On the AST level, as per #545 the expression would
contain a Name and not VarName node that fails the
code generation.

The case only arises in the kernel functions like state_update,
where indexing is "artificially" created with indexing by a Name
only.

fixes #541
iomaganaris pushed a commit that referenced this issue May 10, 2022
Improved index code generation within the LLVM pipeline.
The following issues were addressed:

Array indices are i64 per LLVM's addressing convention.
This means that if the value is not a constant, an additional
sext instruction must be created.

Bounds check is removed since it requires a certain analysis
on the index value. This can be addressed in a separate PR.

`IndexedName` code generation is separated into 2 functions
The first, `get_array_length()` is responsible for array initialisation,
the second, `get_array_index()`, for indexing. In latter case, we
support the following cases:
```
...
// Indexing with an integer constant
k[0] = ...

// Indexing with an integer expression
k[10 - 10] 

// Indexing with a `Name` AST node that is an integer
// (in our case a FOR loop induction variable or a variable
// with `CodegenVarType` == `Integer`
k[id] = ...      
k[ena_id] = ...
```
Note that the case:
```
// id := loop integer induction variable
k[id + 1] = ... 
```
is not supported for 2 reasons:

On the AST level, as per #545 the expression would
contain a Name and not VarName node that fails the
code generation.

The case only arises in the kernel functions like state_update,
where indexing is "artificially" created with indexing by a Name
only.

fixes #541
iomaganaris pushed a commit that referenced this issue May 12, 2022
Improved index code generation within the LLVM pipeline.
The following issues were addressed:

Array indices are i64 per LLVM's addressing convention.
This means that if the value is not a constant, an additional
sext instruction must be created.

Bounds check is removed since it requires a certain analysis
on the index value. This can be addressed in a separate PR.

`IndexedName` code generation is separated into 2 functions
The first, `get_array_length()` is responsible for array initialisation,
the second, `get_array_index()`, for indexing. In latter case, we
support the following cases:
```
...
// Indexing with an integer constant
k[0] = ...

// Indexing with an integer expression
k[10 - 10] 

// Indexing with a `Name` AST node that is an integer
// (in our case a FOR loop induction variable or a variable
// with `CodegenVarType` == `Integer`
k[id] = ...      
k[ena_id] = ...
```
Note that the case:
```
// id := loop integer induction variable
k[id + 1] = ... 
```
is not supported for 2 reasons:

On the AST level, as per #545 the expression would
contain a Name and not VarName node that fails the
code generation.

The case only arises in the kernel functions like state_update,
where indexing is "artificially" created with indexing by a Name
only.

fixes #541
@alkino
Copy link
Member

alkino commented Aug 29, 2022

We should remove VarName at then and use Identifier (base class of all type).

@alkino
Copy link
Member

alkino commented Sep 13, 2022

Related to #903

iomaganaris pushed a commit that referenced this issue Sep 15, 2022
Improved index code generation within the LLVM pipeline.
The following issues were addressed:

Array indices are i64 per LLVM's addressing convention.
This means that if the value is not a constant, an additional
sext instruction must be created.

Bounds check is removed since it requires a certain analysis
on the index value. This can be addressed in a separate PR.

`IndexedName` code generation is separated into 2 functions
The first, `get_array_length()` is responsible for array initialisation,
the second, `get_array_index()`, for indexing. In latter case, we
support the following cases:
```
...
// Indexing with an integer constant
k[0] = ...

// Indexing with an integer expression
k[10 - 10] 

// Indexing with a `Name` AST node that is an integer
// (in our case a FOR loop induction variable or a variable
// with `CodegenVarType` == `Integer`
k[id] = ...      
k[ena_id] = ...
```
Note that the case:
```
// id := loop integer induction variable
k[id + 1] = ... 
```
is not supported for 2 reasons:

On the AST level, as per #545 the expression would
contain a Name and not VarName node that fails the
code generation.

The case only arises in the kernel functions like state_update,
where indexing is "artificially" created with indexing by a Name
only.

fixes #541
iomaganaris pushed a commit that referenced this issue Sep 15, 2022
Improved index code generation within the LLVM pipeline.
The following issues were addressed:

Array indices are i64 per LLVM's addressing convention.
This means that if the value is not a constant, an additional
sext instruction must be created.

Bounds check is removed since it requires a certain analysis
on the index value. This can be addressed in a separate PR.

`IndexedName` code generation is separated into 2 functions
The first, `get_array_length()` is responsible for array initialisation,
the second, `get_array_index()`, for indexing. In latter case, we
support the following cases:
```
...
// Indexing with an integer constant
k[0] = ...

// Indexing with an integer expression
k[10 - 10] 

// Indexing with a `Name` AST node that is an integer
// (in our case a FOR loop induction variable or a variable
// with `CodegenVarType` == `Integer`
k[id] = ...      
k[ena_id] = ...
```
Note that the case:
```
// id := loop integer induction variable
k[id + 1] = ... 
```
is not supported for 2 reasons:

On the AST level, as per #545 the expression would
contain a Name and not VarName node that fails the
code generation.

The case only arises in the kernel functions like state_update,
where indexing is "artificially" created with indexing by a Name
only.

fixes #541
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ast AST related issues bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

4 participants