enh: change the ASR representation of ArrayConstant-2 #3859

Pranavchiku · 2024-04-12T10:53:51Z

| ArrayConstant(int n_data, void data, ttype type, arraystorage storage_format)

I wished to keep PR for both grammar separated, continuation of #3825, both are required.

Pranavchiku · 2024-04-13T06:37:36Z

Integration test and minpack MRE works, lets wait for CI to run.

Pranavchiku · 2024-04-13T06:40:15Z

src/lfortran/semantics/ast_common_visitor.h

+                                            body.reserve(al, ASRUtils::get_fixed_size_of_array(a->m_type));
+                                            for (size_t i = 0; i < (size_t) ASRUtils::get_fixed_size_of_array(a->m_type); i++) {


Suggested change

body.reserve(al, ASRUtils::get_fixed_size_of_array(a->m_type));

for (size_t i = 0; i < (size_t) ASRUtils::get_fixed_size_of_array(a->m_type); i++) {

body.reserve(al, a->m_n_data / 8);

for (size_t i = 0; i < a->m_n_data / 8; i++) {

This can be replaced like this, I'll prefer to do code clean up in subsequent PRs once CI works.

Pranavchiku · 2024-04-13T06:44:43Z

I am not able to reproduce

array14.f90 * run
The JSON metadata differs against reference results
Reference JSON: tests/reference/run-array14-c9f6829.json
Output JSON:    tests/output/run-array14-c9f6829.json
Omitting 10 identical items
Differing items:
{'stdout_hash': 'a91abee346c7fbe1f8cc32e519494cc49325d72c18b672ce1899ff56'} != {'stdout_hash': 'c2c6bd6362cbafef5d23b5ce7119207b9547d521ee1a9889a335a5bc'}
Diff against: tests/reference/run-array14-c9f6829.stdout
1,2c1,2
< 7.00649232e-45           nan 4.20389539e-45 5.60519386e-45 2.80259693e-45 �
< 7.00649232e-45           nan 4.20389539e-45 �
---
> 7.00649232e-45          -nan 4.20389539e-45 5.60519386e-45 2.80259693e-45 �
> 7.00649232e-45          -nan 4.20389539e-45 �

on my device ( mac ).

Pranavchiku · 2024-04-13T08:55:47Z

MRE for asr_to_fortran failure:

program main
    implicit none
    print *, "hello", ["hey", ["xyz", ["abc"]]], [[1, 4, 2, 3], 3, 5, [abs(-2)]], "bye"
end program

% lfortran ./integration_tests/print_arr_03.f90 --show-fortran
program main
implicit none
print *, [4, 9]
print *, [2, -2, 5, 7, 8, -9, 10, 3, 3, -11]
print *, "hello", [heyxyzabc, xyzabc, abc], [1, 4, 2, 3, 3, 5, 2], "bye"
end program main

Pranavchiku · 2024-04-14T06:27:22Z

build failure happens because of:

% cat d.f90 
program array14
    implicit none
    real :: x(5)
    x = [5, -1, 3, 4, 2]
    print *, x
end program
% lfortran d.f90 
7.00649232e-45           nan 4.20389539e-45 5.60519386e-45 2.80259693e-45

Pranavchiku · 2024-04-14T07:02:56Z

I think now the only failures will be Test CPP, Test WASM, Third party code compilation ( hopefully ), will push and get it done.

Pranavchiku · 2024-04-14T07:50:24Z

Here is error for Test CPP

[ 96%] Building CXX object CMakeFiles/asr.dir/serialization.cpp.o
[ 98%] Building CXX object CMakeFiles/asr.dir/utils2.cpp.o
In file included from /home/runner/work/lfortran/lfortran/build/libasr/../libasr/serialization.h:4,
                 from /home/runner/work/lfortran/lfortran/build/libasr/serialization.cpp:4:
/home/runner/work/lfortran/lfortran/build/libasr/../libasr/asr.h: In instantiation of ‘LCompilers::ASR::asr_t* LCompilers::ASR::DeserializationBaseVisitor<Struct>::deserialize_ArrayConstant() [with Struct = LCompilers::ASRDeserializationVisitor]’:
/home/runner/work/lfortran/lfortran/build/libasr/../libasr/asr.h:39793:90:   required from ‘LCompilers::ASR::asr_t* LCompilers::ASR::DeserializationBaseVisitor<Struct>::deserialize_expr() [with Struct = LCompilers::ASRDeserializationVisitor]’
/home/runner/work/lfortran/lfortran/build/libasr/../libasr/asr.h:35758:71:   required from ‘LCompilers::ASR::asr_t* LCompilers::ASR::DeserializationBaseVisitor<Struct>::deserialize_node() [with Struct = LCompilers::ASRDeserializationVisitor]’
/home/runner/work/lfortran/lfortran/build/libasr/serialization.cpp:420:42:   required from here
/home/runner/work/lfortran/lfortran/build/libasr/../libasr/asr.h:38883:31: error: ‘class LCompilers::ASRDeserializationVisitor’ has no member named ‘read_void’; did you mean ‘read_bool’?
38883 |         void *m_data = self().read_void(m_n_data, m_type);
      |                        ~~~~~~~^~~~~~~~~
      |                        read_bool
/home/runner/work/lfortran/lfortran/build/libasr/../libasr/asr.h: In instantiation of ‘void LCompilers::ASR::SerializationBaseVisitor<Struct>::visit_ArrayConstant(const LCompilers::ASR::ArrayConstant_t&) [with Struct = LCompilers::ASRSerializationVisitor]’:
/home/runner/work/lfortran/lfortran/build/libasr/../libasr/asr.h:5013:62:   required from ‘void LCompilers::ASR::visit_expr_t(const LCompilers::ASR::expr_t&, Visitor&) [with Visitor = LCompilers::ASRSerializationVisitor]’
/home/runner/work/lfortran/lfortran/build/libasr/../libasr/asr.h:5226:52:   required from ‘void LCompilers::ASR::BaseVisitor<Struct>::visit_expr(const LCompilers::ASR::expr_t&) [with Struct = LCompilers::ASRSerializationVisitor]’
/home/runner/work/lfortran/lfortran/build/libasr/../libasr/asr.h:5137:43:   required from ‘void LCompilers::ASR::visit_asr_t(const LCompilers::ASR::asr_t&, Visitor&) [with Visitor = LCompilers::ASRSerializationVisitor]’
/home/runner/work/lfortran/lfortran/build/libasr/../libasr/asr.h:5158:49:   required from ‘void LCompilers::ASR::BaseVisitor<Struct>::visit_asr(const LCompilers::ASR::asr_t&) [with Struct = LCompilers::ASRSerializationVisitor]’
/home/runner/work/lfortran/lfortran/build/libasr/serialization.cpp:44:16:   required from here
/home/runner/work/lfortran/lfortran/build/libasr/../libasr/asr.h:34690:16: error: ‘class LCompilers::ASRSerializationVisitor’ has no member named ‘write_void’; did you mean ‘write_bool’?
34690 |         self().write_void(x.m_data, x.m_n_data, x.m_type);
      |         ~~~~~~~^~~~~~~~~~
      |         write_bool
gmake[2]: *** [CMakeFiles/asr.dir/build.make:944: CMakeFiles/asr.dir/serialization.cpp.o] Error 1
gmake[2]: *** Waiting for unfinished jobs....

Pranavchiku · 2024-04-14T09:25:05Z

MRE for dftatom failure:

subroutine get_atomic_states_nonrel(fo)
real, pointer :: fo(:)
allocate(fo(1))
fo = (/ 1 /)
end subroutine

% lfortran a.f90 
Internal Compiler Error: Unhandled exception
Traceback (most recent call last):
  File "/Users/pranavchiku/repos/lfortran/src/bin/lfortran.cpp", line 2402
    LCompilers::print_stack_on_segfault();
  File "/Users/pranavchiku/repos/lfortran/src/bin/lfortran.cpp", line 2366
    err = compile_to_object_file(arg_file, tmp_o, false,
  File "/Users/pranavchiku/repos/lfortran/src/bin/lfortran.cpp", line 906
    result = fe.get_asr2(input, lm, diagnostics);
  File "/Users/pranavchiku/repos/lfortran/src/lfortran/fortran_evaluator.cpp", line 252
    Result<ASR::TranslationUnit_t*> res2 = get_asr3(*ast, diagnostics);
  File "/Users/pranavchiku/repos/lfortran/src/lfortran/fortran_evaluator.cpp", line 274
    compiler_options.symtab_only, compiler_options);
  File "/Users/pranavchiku/repos/lfortran/src/lfortran/semantics/ast_to_asr.cpp", line 107
    if (res.ok) {
  File "/Users/pranavchiku/repos/lfortran/src/lfortran/semantics/ast_body_visitor.cpp", line 3630
    b.is_body_visitor = true;
  File "/Users/pranavchiku/repos/lfortran/src/lfortran/semantics/ast_body_visitor.cpp", line 143
    visit_ast(*x.m_items[i]);
  File "/Users/pranavchiku/repos/lfortran/src/lfortran/ast.h", line 4848
    void visit_ast(const ast_t &b) { visit_ast_t(b, self()); }
  File "/Users/pranavchiku/repos/lfortran/src/lfortran/ast.h", line 4805
    case astType::mod: { v.visit_mod((const mod_t &)x); return; }
  File "/Users/pranavchiku/repos/lfortran/src/lfortran/ast.h", line 4856
    void visit_program_unit(const program_unit_t &b) { visit_program_unit_t(b, self()); }
  File "/Users/pranavchiku/repos/lfortran/src/lfortran/ast.h", line 4447
    case program_unitType::Subroutine: { v.visit_Subroutine((const Subroutine_t &)x); return; }
  File "/Users/pranavchiku/repos/lfortran/src/lfortran/semantics/ast_body_visitor.cpp", line 1952
    transform_stmts(body, x.n_body, x.m_body);
  File "/Users/pranavchiku/repos/lfortran/src/lfortran/semantics/ast_body_visitor.cpp", line 114
    this->visit_stmt(*m_body[i]);
  File "/Users/pranavchiku/repos/lfortran/src/lfortran/ast.h", line 4894
    void visit_stmt(const stmt_t &b) { visit_stmt_t(b, self()); }
  File "/Users/pranavchiku/repos/lfortran/src/lfortran/ast.h", line 4540
    case stmtType::Assign: { v.visit_Assign((const Assign_t &)x); return; }
  File "/Users/pranavchiku/repos/lfortran/src/lfortran/semantics/ast_body_visitor.cpp", line 2456
    args.p, args.n, ASRUtils::type_get_past_allocatable(target_type), ac->m_storage_format)));
  File "/Users/pranavchiku/repos/lfortran/src/libasr/asr_utils.h", line 5381
    int curr_idx = 0;
  File "/Users/pranavchiku/repos/lfortran/src/libasr/asr.h", line 41
    LCOMPILERS_ASSERT(is_a<T>(*f));
AssertFailed: is_a<T>(*f)

Pranavchiku · 2024-04-14T09:44:39Z

With latest commit, I think Test WASM should be the only failure.

Pranavchiku · 2024-04-14T09:58:38Z

I think latest commit will get all tests passed.

Pranavchiku · 2024-04-14T10:03:20Z

CI looks good, all tests shall pass.

Pranavchiku · 2024-04-14T10:24:48Z

There are a few improvements that can be made:

Assure n_data is used as n_data. ( currently script generates it as m_n_data )
if possible void data shall generate void *m_data, int64_t n_data
Use x->n_data to get size of array instead of ASRUtils::get_fixed_size_of_array(x->m_type)
Add ASR verify to assure ASRUtils::get_fixed_size_of_array(x->m_type) == x->n_data * sizeof(void *)
cleanup code

All these are minor except ASR representation, If it is fine, I'll prefer to do it in subsequent PRs.

certik · 2024-04-15T13:30:43Z

src/libasr/bwriter.h

+        for (int64_t i = 0; i < n_data; i++) {
+            write_pointer(p, t, i);
+        }
+    }


Instead of this and the complicated decision making at each element (very slow for large arrays), why cannot we:

make n_data represent the size in bytes

just save n_data bytes of data and that's it.

In ASR verify, add the above check to ensure that n_data is consistent.

Updated this, if you can check and comment if the direction is correct.

src/libasr/asr_verify.cpp

tests/reference/asr-array3-7b1ab3b.stdout

tests/reference/asr-template_array_04-ed63449.stdout

certik · 2024-04-15T13:35:41Z

Let me know how you want to proceed. I think we should fix the size of n_data and do it right first. As long as ASR is correct, then we can merge and the rest we can do in subsequent PRs.

It almost seems like data always represents each element by 8 bytes. Instead, each element must be represented by as many bytes as it would be at runtime (so integer(4) is 4 bytes, not 8), and n_data must be the size in bytes of the allocated data. The read/writer then becomes trivial (no decision making) and fast.

certik · 2024-04-15T13:37:16Z

src/libasr/bwriter.h

+            default:
+                throw LCompilersException("Unsupported type for array constant.");
+        }
+    }


None of these decisions should be here. Just read n_data of bytes into data, and that's it.

Pranavchiku · 2024-04-15T13:37:28Z

I think it needs to be addressed in this PR, I have it listed in tracker above comments. I’ll do it by tonight.

certik

Awesome, great job!

Thanks for getting this done. I think it is good enough to be merged.

After merging, let's print the array contents in the ASR clojure printouts, so that we can see what values are in the ConstantArrays for debugging.

certik · 2024-04-16T21:24:50Z

tests/reference/asr-array10-1f60f5f.stdout

-                                        (IntegerConstant 6 (Integer 4))
-                                        (IntegerConstant 12 (Integer 4))
-                                        (IntegerConstant 13 (Integer 4))]
+                                        20


Sure. Open up an issue for this.

gxyd · 2024-04-17T11:31:18Z

src/libasr/ASR.asdl

    | FunctionParam(int param_number, ttype type, expr? value)
    | ArrayConstructor(expr* args, ttype type, expr? value, arraystorage storage_format)
-    | ArrayConstant(expr* args, ttype type, arraystorage storage_format)
+    | ArrayConstant(int n_data, void data, ttype type, arraystorage storage_format)


I'm sorry, but what is n_data supposed to be? I'm not able to fully understand it. Let's say we've:

["ap", "ba", "ca"]

what is n_data supposed to be for that, 3?

It represents bytes occupied by ArrayConstant. In this case it will be 2*3 = 6

Makes sense. Though another part is with regards to the ASR changes, for e.g. for the below Fortran:

program main print *, [character(2):: "ap", "bp", "cd"] end program

the ASR earlier used to look like:

(TranslationUnit (SymbolTable 1 { main: (Program (SymbolTable 2 { }) main [] [(Print [(ArrayConstant [(StringConstant "ap" (Character 1 2 ()) ) (StringConstant "bp" (Character 1 2 ()) ) (StringConstant "cd" (Character 1 2 ()) )] (Array (Character 1 2 ()) [((IntegerConstant 1 (Integer 4)) (IntegerConstant 3 (Integer 4)))] FixedSizeArray ) ColMajor )] () () )] ) }) [] )

now, it looks like:

(TranslationUnit (SymbolTable 1 { main: (Program (SymbolTable 2 { }) main [] [(Print [(ArrayConstant 6 (Array (Character 1 2 ()) [((IntegerConstant 1 (Integer 4)) (IntegerConstant 3 (Integer 4)))] FixedSizeArray ) ColMajor )] () () )] ) }) [] )

the program executes just fine, but curious for the missing "ap", "bp", "cd" information and if that's intentional.

Now void* m_data points to ["ap", "bp", "cd"]

You may wish to read #3581, it will make things clear.

Ah, yes I see you already have an open issue for it, sorry to have bugged you.

Can you try following example with #3891 and see if it works :)

Yes, seems to be working quite well. 👍

Pranavchiku mentioned this pull request Apr 12, 2024

enh: change the ASR representation of ArrayConstant #3825

Closed

Pranavchiku force-pushed the gh3581-2 branch from f248664 to f7a38d7 Compare April 13, 2024 06:37

Pranavchiku commented Apr 13, 2024

View reviewed changes

Pranavchiku mentioned this pull request Apr 14, 2024

OpenMP support in AST and ASR #3777

Open

4 tasks

Pranavchiku force-pushed the gh3581-2 branch 2 times, most recently from 306f680 to 9b030d0 Compare April 14, 2024 09:11

Pranavchiku added the enhancement Issues or pull request for enhancing existing functionality label Apr 14, 2024

Pranavchiku marked this pull request as ready for review April 14, 2024 10:04

Pranavchiku added the asr ASR Related changes label Apr 14, 2024

Pranavchiku force-pushed the gh3581-2 branch from 6c79b3c to db79207 Compare April 14, 2024 10:36

Pranavchiku requested a review from certik April 15, 2024 05:04

certik reviewed Apr 15, 2024

View reviewed changes

src/libasr/asr_verify.cpp Show resolved Hide resolved

certik reviewed Apr 15, 2024

View reviewed changes

tests/reference/asr-array3-7b1ab3b.stdout Outdated Show resolved Hide resolved

certik reviewed Apr 15, 2024

View reviewed changes

tests/reference/asr-template_array_04-ed63449.stdout Outdated Show resolved Hide resolved

certik marked this pull request as draft April 15, 2024 13:35

certik reviewed Apr 15, 2024

View reviewed changes

Pranavchiku marked this pull request as ready for review April 16, 2024 17:48

certik approved these changes Apr 16, 2024

View reviewed changes

Pranavchiku added 22 commits April 16, 2024 15:28

feat: introduce void data in ArrayConstant node

b2e5bd7

enh: handle void in asdl.py and asdl_cpp.py

9ee77ee

enh: setup helper functions, handle correctly creation of array constant

c82ad35

chore: consequent changes in codebase

2dcb7f2

chore: apply code review

95989f0

feat: introduce n_data in ArrayConstant node

8704b1f

chore: consequent changes

267bbdf

enh: setup serialization and deserialization of ArrayConstant

d81aad2

test: update references

34139ff

fix: character handling while fetching array constant value

ed03447

fix: incorrect casting of array values in assignment

145d4ad

test: update references

707022b

fix: read_void, write_void error in test cpp

a57aa04

fix: handling of type pointer for dftatom failure

f3b796b

fix: support all kind of bool while deserializing

e1e2f3a

enh: support arrayconstant in asr to wasm

6abbb94

enh: set n_data as suggested and verify it

efd622c

test: update references

5b0c6c9

enh: simplify write/reader code

2e6be12

chore: set bytes of characer correctly, and rearrange asr.asdl

8ca5cbb

chore: use 1 byte to represent in write / reader, clean up reader/writer

d009546

test: update references

708328b

certik force-pushed the gh3581-2 branch from 0e9e70e to 708328b Compare April 16, 2024 21:28

certik enabled auto-merge April 16, 2024 21:28

certik merged commit 23ae9a5 into lfortran:main Apr 16, 2024

This was referenced Apr 17, 2024

enh: add support to print ConstantArrays #3885

Closed

refactor ArrayConstant code #3886

Open

gxyd reviewed Apr 17, 2024

View reviewed changes

		body.reserve(al, ASRUtils::get_fixed_size_of_array(a->m_type));
		for (size_t i = 0; i < (size_t) ASRUtils::get_fixed_size_of_array(a->m_type); i++) {

Uh oh!

enh: change the ASR representation of ArrayConstant-2 #3859

enh: change the ASR representation of ArrayConstant-2 #3859

Uh oh!

Conversation

Pranavchiku commented Apr 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Pranavchiku commented Apr 13, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Pranavchiku commented Apr 13, 2024

Uh oh!

Pranavchiku commented Apr 13, 2024

Uh oh!

Pranavchiku commented Apr 14, 2024

Uh oh!

Pranavchiku commented Apr 14, 2024

Uh oh!

Pranavchiku commented Apr 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Pranavchiku commented Apr 14, 2024

Uh oh!

Pranavchiku commented Apr 14, 2024

Uh oh!

Pranavchiku commented Apr 14, 2024

Uh oh!

Pranavchiku commented Apr 14, 2024

Uh oh!

Pranavchiku commented Apr 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

certik commented Apr 15, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Pranavchiku commented Apr 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

certik left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gxyd Apr 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Pranavchiku Apr 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Pranavchiku commented Apr 12, 2024 •

edited

Loading

Pranavchiku commented Apr 14, 2024 •

edited

Loading

Pranavchiku commented Apr 14, 2024 •

edited

Loading

Pranavchiku commented Apr 15, 2024 •

edited

Loading

gxyd Apr 17, 2024 •

edited

Loading

Pranavchiku Apr 17, 2024 •

edited

Loading