lcompilers / lpython

Python compiler
https://lpython.org/
Other
1.5k stars 157 forks source link

ASR: Support array storage order in memory - row major or col major #1249

Open Shaikh-Ubaid opened 1 year ago

Shaikh-Ubaid commented 1 year ago

First encountered here https://github.com/lcompilers/lpython/pull/1118#discussion_r973492701

https://github.com/lcompilers/lpython/blob/8ec42d225a08f5b3d6bce9dd68680ddabdfb8a6c/src/libasr/codegen/asr_to_wasm.cpp#L1432-L1442

Currently, when syncing lpython and lfortran libasr's, we are needing to keep track of this. Both the libasr's slightly diverge due to it.

Shaikh-Ubaid commented 1 year ago

There is a better approach suggested by Ondrej.


    It seems it should rather be part of the type. Here is one way to do it cleanly:
--- a/src/libasr/ASR.asdl
+++ b/src/libasr/ASR.asdl
@@ -327,22 +327,23 @@ expr
 --     integer, so we also use kind=4 for the default logical.)

 ttype
-    = Integer(int kind, dimension* dims)
-    | Real(int kind, dimension* dims)
-    | Complex(int kind, dimension* dims)
-    | Character(int kind, int len, expr? len_expr, dimension* dims)
-    | Logical(int kind, dimension* dims)
+    = Integer(int kind)
+    | Real(int kind)
+    | Complex(int kind)
+    | Character(int kind, int len, expr? len_expr)
+    | Logical(int kind)
     | Set(ttype type)
     | List(ttype type)
     | Tuple(ttype* type)
-    | Struct(symbol derived_type, dimension* dims)
-    | Enum(symbol enum_type, dimension *dims)
-    | Union(symbol union_type, dimension *dims)
-    | Class(symbol class_type, dimension* dims)
+    | Struct(symbol derived_type)
+    | Enum(symbol enum_type)
+    | Union(symbol union_type)
+    | Class(symbol class_type)
     | Dict(ttype key_type, ttype value_type)
     | Pointer(ttype type)
+    | Array(ttype type, dimension* dims, arraystorage storage_format)
     | CPtr()
-    | TypeParameter(identifier param, dimension* dims)
+    | TypeParameter(identifier param)

 restriction_arg = RestrictionArg(identifier restriction_name, symbol restriction_func)

But that requires more refactoring, so we can do it in a separate PR.

Originally posted by @certik in https://github.com/lcompilers/lpython/issues/1252#issuecomment-1300983863

certik commented 1 year ago

@czgdp1807 another argument to possibly use Array ASR node.

czgdp1807 commented 1 year ago

The refactoring makes sense. The question is when do you want to do it?

certik commented 1 year ago

After we deliver on Minpack. We are close, don't want to delay it.

certik commented 1 year ago

Solution how to do this: https://github.com/lfortran/lfortran/issues/1601, the row/column major becomes part of the physical type.