enh: support `compiletime_broadcast_elemental_intrinsic` for multiple arguments #3684

gxyd · 2024-03-19T18:25:32Z

certik · 2024-03-19T18:31:03Z

src/libasr/asr_utils.h

    for (auto &a : args) {
+        if (allow_nullptr && !a) continue;


I am not sure about this change --- when can an argument here in the vector be null?

For something like: floor(1.2) where the second optional argument is set to nullptr.

Ok. Let's get all tests passing, then I'll review one more time.

gxyd · 2024-03-20T12:44:52Z

@certik , could you help me understand if the failing CI jobs are related to my changes?

the error says:

std::allocator<unsigned char> >, void>, bool)':
2024-03-20T10:25:01.8765884Z /home/runner/work/lfortran/lfortran/lfortran-0.33.1-866-ged79bd5c3/src/lfortran/fortran_kernel.cpp:346: undefined reference to `xeus::xinterpreter::publish_execution_result(int, nlohmann::json_abi_v3_11_3::basic_json<std::map, std::vector, std::__cxx11::basic_string<char, std::char_traits<char>,
 std::allocator<char> >, bool, long, unsigned long, double, std::allocator, nlohmann::json_abi_v3_11_3::adl_serializer, std::vector<unsigned char, std::allocator<unsigned char> >, void>, nlohmann::json_abi_v3_11_3::basic_json<std::map, std::vector, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, bool, long, unsigned long, 
double, std::allocator, nlohmann::json_abi_v3_11_3::adl_serializer, std::vector<unsigned char, std::allocator<unsigned char> >, void>)'

Not able to understand how to go about this.

while the failing documentation job gives the error:

/usr/bin/ld: /home/runner/work/lfortran/lfortran/src/lfortran/fortran_kernel.cpp:511: undefined reference to `xeus::xkernel::xkernel(xeus::xconfiguration const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::unique_ptr<xeus::xcontext, std::default_delete<xeus::xcontext> >, 
std::unique_ptr<xeus::xinterpreter, std::default_delete<xeus::xinterpreter> >, std::unique_ptr<xeus::xserver, std::default_delete<xeus::xserver> > (*)(xeus::xcontext&, xeus::xconfiguration const&,
 nlohmann::json_abi_v3_11_3::detail::error_handler_t), std::unique_ptr<xeus::xhistory_manager, std::default_delete<xeus::xhistory_manager> >, std::unique_ptr<xeus::xlogger, std::default_delete<xeus::xlogger> >, std::unique_ptr<xeus::xdebugger, std::default_delete<xeus::xdebugger> > (*)(xeus::xcontext&, xeus::xconfiguration
 const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, nlohmann::json_abi_v3_11_3::basic_json<std::map, std::vector, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, bool, long, unsigned long, double, std::allocator, nlohmann::json_abi_v3_11_3::adl_serializer,
 std::vector<unsigned char, std::allocator<unsigned char> >, void> const&), nlohmann::json_abi_v3_11_3::basic_json<std::map, std::vector, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, bool, long, unsigned long, double, std::allocator, nlohmann::json_abi_v3_11_3::adl_serializer, std::vector<unsigned char, std::allocator<unsigned char> >, void>, nlohmann::json_abi_v3_11_3::detail::error_handler_t)'

certik · 2024-03-20T14:23:11Z

It doesn't seem like triggered by your PR, but something is wrong, it's the same error on all 3 platforms.

Can you try submitting a simple PR to check if master passes?

certik · 2024-03-20T14:34:29Z

The CI failure is unrelated: #3687.

gxyd · 2024-03-21T08:30:56Z

This is ready for a review.

Pranavchiku · 2024-03-21T13:21:16Z

src/lfortran/semantics/ast_common_visitor.h

+                    // 2. or arg.size() is 2 and first arg is an array, and second arg "kind" is
+                    //   distributed to each broadcasted call
+                    // 
+                    if ((args.size() == 2 || args.size() == 1) &&


I do not think we need this now, we shall go ahead and modify signature to

- void compiletime_broadcast_elemental_intrinsic(ASR::ArrayConstant_t* array, ASRUtils::create_intrinsic_function create_func, const Location& loc, Allocator& al) + void compiletime_broadcast_elemental_intrinsic(ASR::expr_t** args, ASRUtils::create_intrinsic_function const Location& loc, Allocator& al)

Now iterate over args, and store in std::vector<int> array_arg_indices. Once we populate array_arg_indices, for example it has {1, 2, 4} and index {0, 3, 5} are scalar. We will create a helper funciton that takes array_arg_indices and all other necessary parameters and broadcast X(s0, a1, a2, s3, a4, s5 as [X(s0, a1[0], a2[0], s3, a4[0], s5), ...., ].

The current approach handles only kind parameter and limits us to only two arguments ( same as main with 1 argument ).

Could you please give an example of an intrinsic function where broadcasting in this way would be needed? i.e. some being arrays, others being scalars, and it's an elemental intrinsic procedure? I'm just not able to think of something.

We can also have multiple array arguments

program main print *, max([1.0, 2.0, 3.0], [4.0, 5.0, 6.0]) end program

Pranavchiku · 2024-03-21T13:27:45Z

src/libasr/asr_utils.h

    for (auto &a : args) {
+        if (ignore_null && !a) continue;


We do not need this because in intrinsic_functions.h it is already taken care that there will not be any nullptr. For frontend this happens because we fill_optional_kind_arg and cases like max, min, etc which has multiple arguments.

So, you are saying that we can remove call to fill_optional_kind_arg as well as remove the ignore_null extra added keyword argument as well from all_args_evaluated?

No, no, we need fill_optional_kind_arg, just check all_args_evaluated before filling optional arguments with nullptr.

No, no, we need fill_optional_kind_arg, just check all_args_evaluated before filling optional arguments with nullptr.

I thought about this, and even if I do it before fill_optional_kind_arg as you mentioned that it would be filled with nullptr (for e.g. for min) that will happen even before fill_optional_kind_arg, as args is populated with nullptr in handle_intrinsic_node_args itself.

Pranavchiku

Left some comments above, let me know if you wish more information about anything. I am marking this as draft.

Pranavchiku · 2024-03-21T13:30:55Z

The approach in this PR works well, no doubts, that looks good as well. Just if we are fixing it, and there is no hurry at moment ( it is not a blocker afaik ), let us do it correctly. I'll be happy to help.

…asting fixes lfortran#3673 and write test broadcasting test cases for elemental functions

gxyd · 2024-03-22T11:03:22Z

Let me know how it looks now @Pranavchiku

certik · 2024-03-22T14:40:41Z

integration_tests/intrinsics_195.f90

+    print *, max(1, [-1, 2, 20])
+    print *, max([1, 2, 3], [1, 1, [2]])
+    print *, max([1, 2], -1, -4)
+end program


These need to have tests like if (...) error stop to ensure the answer is correct.

Test shape and size of the resulting array also.

Ah yes, I should've added it in the correct way, I'll change that.

certik · 2024-03-22T14:43:47Z

integration_tests/intrinsics_194.f90

+    print *, min([-1, 2, 3], 2, 5, [4, 4, 5], [5, -8, 7])
+    print *, min(1, [-1, 2, 20])
+    print *, min([1, 2, 3], [1, 1, [2]])
+    print *, min([1, 2], -1, -4)


This prints with GFortran:

-1 -8 2 -1 1 1 1 1 2 -4 -4

How does this work? I don't understand what min does with a combination of array and scalar arguments like this.

I think it extends all arguments to be arrays like this:

min([-1, 2, 3], 2, 5, [4, 4, 5], [5, -8, 7]) -> min([-1, 2, 3], [2, 2, 2], [5, 5, 5], [4, 4, 5], [5, -8, 7])

And then it computes the minimum element-wise, so we get [-1, -8, 2]?

Yes, that's correct.

certik · 2024-03-22T14:47:12Z

@gxyd thanks for working on this. I think there are many bugs like these and we need to fix them all, so this is extremely useful.

gxyd · 2024-03-22T16:24:01Z

I think there are many bugs like these

@certik are there open issues for such bugs? (couldn't find any similar to this), let me know if there are more similar issues which need attention, I could take a look at those if needed.

certik · 2024-03-22T19:47:13Z

Yes, here are some:

Pranavchiku · 2024-03-23T03:04:13Z

I’ll review this PR in a while, around 11:30 IST.

Pranavchiku

This is good, thanks for doing this!

certik reviewed Mar 19, 2024

View reviewed changes

gxyd force-pushed the broadcast_intrinsic_functions branch from eb6ff65 to d8faf29 Compare March 20, 2024 10:15

gxyd force-pushed the broadcast_intrinsic_functions branch 3 times, most recently from c1e0b6a to 4772a8a Compare March 21, 2024 07:13

Pranavchiku reviewed Mar 21, 2024

View reviewed changes

Pranavchiku requested changes Mar 21, 2024

View reviewed changes

Pranavchiku marked this pull request as draft March 21, 2024 13:29

gxyd added 3 commits March 22, 2024 13:56

improve 'compiletime_broadcast_elemental_intrinsic' to support broadc…

0ec6cfe

…asting fixes lfortran#3673 and write test broadcasting test cases for elemental functions

change in approach to allow more args

f818954

throw semantic error on invalid shape

cdf5193

gxyd force-pushed the broadcast_intrinsic_functions branch from db2f763 to cdf5193 Compare March 22, 2024 08:31

gxyd added 3 commits March 22, 2024 14:11

add test files correctly

bd630e7

fix error by extracting_value

fd15176

assign m_type only once

fbd63ae

gxyd marked this pull request as ready for review March 22, 2024 10:50

certik reviewed Mar 22, 2024

View reviewed changes

gxyd added 2 commits March 22, 2024 21:37

add some more tests and improve other test cases

3e94391

add example of broadcasting as docstring

670293b

certik requested a review from Pranavchiku March 22, 2024 19:46

Pranavchiku approved these changes Mar 23, 2024

View reviewed changes

Pranavchiku changed the title ~~improve 'compiletime_broadcast_elemental_intrinsic' to support broadcasting for intrinsics~~ enh: support compiletime_broadcast_elemental_intrinsic for multiple arguments Mar 23, 2024

Pranavchiku merged commit f63bb12 into lfortran:main Mar 23, 2024
21 checks passed

gxyd deleted the broadcast_intrinsic_functions branch March 25, 2024 06:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

enh: support `compiletime_broadcast_elemental_intrinsic` for multiple arguments #3684

enh: support `compiletime_broadcast_elemental_intrinsic` for multiple arguments #3684

gxyd commented Mar 19, 2024

certik Mar 19, 2024

gxyd Mar 19, 2024

certik Mar 19, 2024

gxyd commented Mar 20, 2024

certik commented Mar 20, 2024

certik commented Mar 20, 2024

gxyd commented Mar 21, 2024

Pranavchiku Mar 21, 2024

Pranavchiku Mar 21, 2024

gxyd Mar 21, 2024

Pranavchiku Mar 21, 2024

Pranavchiku Mar 21, 2024

gxyd Mar 21, 2024

Pranavchiku Mar 21, 2024 •

edited

gxyd Mar 22, 2024

Pranavchiku left a comment

Pranavchiku commented Mar 21, 2024 •

edited

gxyd commented Mar 22, 2024

certik Mar 22, 2024

certik Mar 22, 2024

gxyd Mar 22, 2024

certik Mar 22, 2024

certik Mar 22, 2024

gxyd Mar 22, 2024

certik commented Mar 22, 2024

gxyd commented Mar 22, 2024 •

edited

certik commented Mar 22, 2024 •

edited

Pranavchiku commented Mar 23, 2024

Pranavchiku left a comment

enh: support compiletime_broadcast_elemental_intrinsic for multiple arguments #3684

enh: support compiletime_broadcast_elemental_intrinsic for multiple arguments #3684

Conversation

gxyd commented Mar 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gxyd commented Mar 20, 2024

certik commented Mar 20, 2024

certik commented Mar 20, 2024

gxyd commented Mar 21, 2024

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

Pranavchiku Mar 21, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pranavchiku left a comment

Choose a reason for hiding this comment

Pranavchiku commented Mar 21, 2024 • edited

gxyd commented Mar 22, 2024

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

certik commented Mar 22, 2024

gxyd commented Mar 22, 2024 • edited

certik commented Mar 22, 2024 • edited

Pranavchiku commented Mar 23, 2024

Pranavchiku left a comment

Choose a reason for hiding this comment

enh: support `compiletime_broadcast_elemental_intrinsic` for multiple arguments #3684

enh: support `compiletime_broadcast_elemental_intrinsic` for multiple arguments #3684

Pranavchiku Mar 21, 2024 •

edited

Pranavchiku commented Mar 21, 2024 •

edited

gxyd commented Mar 22, 2024 •

edited

certik commented Mar 22, 2024 •

edited