Remove arr printing in pass `print_arr` #5039

assem2002 · 2024-10-13T20:33:08Z

Fixes #4860
Fixes #4798
Fixes #4719
Fixes #4523
Fixes ##5739 (The issue now complains about wrong printing with provided formmating)
towards ##5672

tests/reference/asr-int_01-9bf3eb9.stderr

certik · 2024-10-14T15:34:16Z

The CI currently fails.

certik · 2024-11-26T14:51:59Z

@assem2002 let's finish this?

assem2002 · 2024-11-27T08:09:02Z

I'll put it as next to work on. It's tricky, but It will be good to have this issue resolved.

assem2002 · 2024-11-27T08:13:58Z

The major issue is with the various backends we support. This PR removes the implementation in print_arr.cpp pass that iterates on the array and print it element by element in the ASR, instead the llvm backend can handle such a thing and that how things go with write it uses the backend printing not the ASR.
The major CI fails happen because we don't support array printing in the backend with c++ and WASM. Can we make the pass use the removed implementation with all other backends but LLVM? atleast for now

certik · 2024-11-28T05:20:23Z

You can keep the old pass and run it for other backends, but add a new pass or whatever new way just for the LLVM backend. That way things wouldn't change for the other backends and we can merge it.

…onflicts now with array_size of kind(8)

…(A workaround - The codebase doesn't handle array size of integer kind 8)

…operating on

…asm namespace

…of kind 4

… It has some weird bug with dead code llvm optimization

assem2002 · 2024-12-10T08:24:31Z

The failing test (https://github.com/lfortran/lfortran/actions/runs/12251263956/job/34176739756) works for me locally. It's usually related to the optimization done by llvm, It considers some of the ASR is dead code and removes it and sometimes segfaults. I think NOFAST label should be good for now, I believe we can later get that fixed by fixing a similar issue like that one #5062 -- I believe it has the same issue somehow.

assem2002 · 2024-12-10T08:42:33Z

I test all the issues linked to this PR and they work as intended.

certik

Thanks for the fixes! I think there are some design choices that I think we should do differently, I left comments. Let me know what you think.

Let's also add tests that previously segfaulted but now work.

Why does it fail with FAST?

certik · 2024-12-10T14:33:50Z

src/libasr/runtime/lfortran_intrinsics.c

@@ -783,7 +783,7 @@ char** parse_fortran_format(char* format, int64_t *count, int64_t *item_start) {

 struct array_iteration_state{
    //Preserve array size and current element index
-    int64_t array_size;
+     int32_t array_size;


Why did we move from 64bit to 32bit?

32bit is faster, and it's ok to use by default, but if we only support 1 integer size, I think we should support 64bit, otherwise we can't index larger arrays than 2GB, so it is going to bite us later.

I think we either support just one int, in that case it must be 64 bit. Or we support both, then the default can be 32 bit for speed, as long as users can select 64 bit.

Yeah I think it kinda of important thing to handle. We have issue with handling size node with return type of kind 8 . I'll invoke the error and let you know and maybe create and MRE and report the issue. I would like also to mention that allocatable arrays don't handle size of i64 the array_descriptor just have i32.

I addressed the issue here #5647. I'm working on it.

certik · 2024-12-10T14:35:41Z

src/libasr/pass/print_arr.cpp

+                                                ASR::is_a<ASR::StructType_t>(*
+                                                ASRUtils::type_get_past_array_pointer_allocatable(
+                                                ASRUtils::expr_type(format->m_args[i])));
+                if (PassUtils::is_array(format->m_args[i]) && print_not_handled_by_backend ) {


This is problematic. I think we should not be hardwiring backend information into ASR passes. I think ASR passes should not care what current and future backends you use them with.

Rather, we want to pass in the option to either transform the array or not, as a regular option into the pass constructor. Then at the upper level you decide what passes and what options to call.

That's great thing to know about. So you suggest passing a flag to the pass saying (okay we support printing array in the backend we're targeting), rather than making the passes themselves try to make decisions based on the passed backend?

Yes, exactly. You tell the ASR pass exactly what you want to get done. If we are not transforming the printing, is the print_arr pass even doing anything? If so, just pass in a flag to customize what it is doing.

I just want to make sure we're on the same page. I'll just create a bool backend_supports_array_printing=false as member of the PassOptions struct and the print_arr acts upon that flag. Is that correct?

I would call the flag differently. I don't quite understand what print_arr is doing if not changing the array printing. Why not just skip the pass itself altogether? If you can explain to me what else the pass is doing, I can provide a better direction.

If we need the pass, then the option should be call "skip_array_printing". The pass does not care about any backend, nor what it supports.

Regarding the PassOptions, you can probably put it there for now.

for now we support printing arrays in LLVM for fortran primitive types (character, integer, real, complex, ...) but not for arrays of type struct (Fortran type). for any other backend we don't yet support printing arrays in the backend.
So, We need to always use the pass with all the backends; but with LLVM we have to use the pass for only the case of array of type struct (Fortran type).

The actual motive behind printing arrays in the backend instead of using the ASR was returning correct output when using fmt as it could be runtime value so we had to print the array at the runtime instead of replacing the print node for the whole array with arrayitems, so we gave everything to the backend (the fmt and all the arguments) to handle the formatting at runtime.

now I'm questioning the road we went through! Direct array printing statement seems to be a thing to be handled by the frontend of the compiler as we could be using low-level backend like LLVM or C or C++ that doesn't know how to print an array. but we still face the issue of the runtime formatting on the args, The ASR can't spread the array into separate printable elements for the string_format function to recognize -- print_arr pass loops on the array based on the runtime size of the array.

We need to find a way that makes the runtime-formatting function match against the actual element (runtime element) but also to not make the backend really concerned about proper looping and knowing the type of the thing it's trying to print ( EX : An array of struct A ( struct A has struct B as member), this would be a messy message to pass and it could get so long and very nested)

certik · 2024-12-10T14:37:06Z

src/libasr/utils.h

@@ -61,6 +65,7 @@ struct PassOptions {
    bool openmp = false;
    bool experimental_simplifier = false;
    bool enable_gpu_offloading = false;
+    Backend backend = Backend::llvm;


I think this is problematic. I think we don't care in the compiler options which backends we use. For example, users can write their own backends, and they want to call certain passes. We should not hardwire backends like this, nor make any decisions based on what backends are run.

Rather, the compilation is composed of various passes with options, and users assemble their compilation pipeline as they see fit.

certik · 2024-12-10T14:37:33Z

src/bin/lfortran.cpp


-enum Backend {
-    llvm, c, cpp, x86, wasm, fortran, mlir
-};


I think this should stay here. It's the highest-level driver that should decide what pipelines and backends will get run. The internals of the compiler should not care.

If that is #5039 (comment) what we should do, so this change won't be needed.

assem2002 commented Oct 13, 2024

View reviewed changes

tests/reference/asr-int_01-9bf3eb9.stderr Outdated Show resolved Hide resolved

assem2002 marked this pull request as ready for review October 13, 2024 20:57

certik marked this pull request as draft October 14, 2024 15:34

assem2002 mentioned this pull request Oct 15, 2024

intrinsics_316_FAST fails in Ubuntu #5062

Open

assem2002 mentioned this pull request Nov 26, 2024

Incorrect printing of complex arrays #5490

Open

assem2002 force-pushed the Remove-arr-printing branch 2 times, most recently from e833905 to 698dcea Compare December 9, 2024 01:26

assem2002 added 8 commits December 10, 2024 08:20

Change kind of returning array size to kind(4) as codebase has some c…

a59bf2b

…onflicts now with array_size of kind(8)

Change compile_time_size expression type to be an integer of kind 4 …

9884353

…(A workaround - The codebase doesn't handle array size of integer kind 8)

Add backend enum variable in pass_options to know what backend we're …

3cf2261

…operating on

Print arrays using ASR if 'print_not_handled_by_backend' flag is true

f5c90e1

minor : enclose the enum using 'enum class' to avoid collision with w…

a46ac28

…asm namespace

Update reference tests

1cfcb7f

hange array_size to int32 as we now create the size node with return …

488bf03

…of kind 4

Add noFast flag on where_10 (The test already returns wrong results),…

89cbab5

… It has some weird bug with dead code llvm optimization

assem2002 force-pushed the Remove-arr-printing branch from 698dcea to 89cbab5 Compare December 10, 2024 07:19

assem2002 marked this pull request as ready for review December 10, 2024 08:24

Add nofast flag on test

6eb7d2e

certik reviewed Dec 10, 2024

View reviewed changes

certik marked this pull request as draft December 10, 2024 14:39

assem2002 mentioned this pull request Dec 17, 2024

Automatic reallocation failed #5739

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove arr printing in pass `print_arr` #5039

Remove arr printing in pass `print_arr` #5039

assem2002 commented Oct 13, 2024 •

edited

Loading

certik commented Oct 14, 2024

certik commented Nov 26, 2024

assem2002 commented Nov 27, 2024

assem2002 commented Nov 27, 2024 •

edited

Loading

certik commented Nov 28, 2024

assem2002 commented Dec 10, 2024 •

edited

Loading

assem2002 commented Dec 10, 2024

certik left a comment

certik Dec 10, 2024

assem2002 Dec 10, 2024

assem2002 Dec 11, 2024

certik Dec 10, 2024

assem2002 Dec 10, 2024

certik Dec 10, 2024

assem2002 Dec 11, 2024 •

edited

Loading

certik Dec 11, 2024

assem2002 Dec 12, 2024

assem2002 Dec 12, 2024 •

edited

Loading

certik Dec 10, 2024

certik Dec 10, 2024

assem2002 Dec 10, 2024

Remove arr printing in pass print_arr #5039

Are you sure you want to change the base?

Remove arr printing in pass print_arr #5039

Conversation

assem2002 commented Oct 13, 2024 • edited Loading

certik commented Oct 14, 2024

certik commented Nov 26, 2024

assem2002 commented Nov 27, 2024

assem2002 commented Nov 27, 2024 • edited Loading

certik commented Nov 28, 2024

assem2002 commented Dec 10, 2024 • edited Loading

assem2002 commented Dec 10, 2024

certik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

assem2002 Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

assem2002 Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Remove arr printing in pass `print_arr` #5039

Remove arr printing in pass `print_arr` #5039

assem2002 commented Oct 13, 2024 •

edited

Loading

assem2002 commented Nov 27, 2024 •

edited

Loading

assem2002 commented Dec 10, 2024 •

edited

Loading

assem2002 Dec 11, 2024 •

edited

Loading

assem2002 Dec 12, 2024 •

edited

Loading