Skip to content

Conversation

@lucifer1004
Copy link
Member

@lucifer1004 lucifer1004 commented Sep 22, 2022

Tasks

To be implemented

  • distinguish integer and float division
  • use information in ExternalSymbol to create using statements
  • handle operator precedence and associativity according to https://docs.julialang.org/en/v1/manual/mathematical-operations/#Operator-Precedence-and-Associativity
  • FunctionCall
  • convert_variable_decl with type information kept (currently type info is discarded)
  • Array-related functions
  • Better handling of Refs
  • Use OffsetArray to handle custom-indexed Fortran arrays
  • visit_Return
  • visit_Goto and visit_GotoTarget using Julia's @goto and @label
  • allocate and deallocate
  • assert
  • visit_Exit, visit_Cycle, visit_Stop, visit_ErrorStop
  • visit_DoConcurrentLoop
  • visit_Associate
  • visit_Interface

To be tested

  • Operator precedence
  • Array operations
  • Goto statements

Known issues

  • integration_tests/arrays_09.f90: a(1)(4:9) becomes a[1].

@certik
Copy link
Contributor

certik commented Sep 23, 2022

Thanks for starting it!

One design question is whether it should create Julia jl_expr_t expressions, or rather just Julia source code.

The advantage of just creating Julia source code (as a string) is that we do not depend on Julia and it should be easier to create, maintain and debug, and one can also use this to create Julia sources that can be checked in. The advantage of returning jl_expr_t is that it can be evaluated in Julia right away. I don't know what the performance pros/cons are exactly. Let me know your requirements / motivations for using jl_expr_t over just source code.

@lucifer1004
Copy link
Member Author

By using jl_expr_t,

  • We do not need to handle code formats (e.g., indent).
  • We can still emit plain text (via Julia), which is guaranteed to be syntactically correct.

Dependency on Julia, in my mind, is not a big issue, since most people who want to translate Fortran into Julia should have Julia installed.

@certik
Copy link
Contributor

certik commented Sep 23, 2022

I see. We can try this and see how complicated it is. It might be more work to use the Julia API than just emitting strings, but maybe it's not. If you like this approach, we can try it and see.

@lucifer1004
Copy link
Member Author

lucifer1004 commented Sep 23, 2022

Seems that the jl_expr_t route causes LLVM double-linking issues since the Julia dynamic lib is linked to another version of LLVM.

@lucifer1004
Copy link
Member Author

I don't know if this issue can be handled easily. If not, then we will have to switch to the string route.

@awvwgk
Copy link
Member

awvwgk commented Sep 23, 2022

I don't know if this issue can be handled easily

Only if LFortran can be linked against the patched LLVM provided by Julia. I think there are some undertaking in the Julia repository to build against an unpatched LLVM (i.e. upstreaming their patches), but this seems to require LLVM 13/14/HEAD depending on platform/architecture.

Note that unless the LLVM version provided by Julia is 11, #291 will be relevant.

@lucifer1004
Copy link
Member Author

I think it is better to emit Julia source code directly, at least for now.

@certik
Copy link
Contributor

certik commented Sep 24, 2022

For the transpiling use case, you can also build LFortran without LLVM.

However, emitting strings seems simpler and more robust.

@lucifer1004 lucifer1004 changed the title Set up Julia dev environment Translate to Julia Sep 24, 2022
@lucifer1004 lucifer1004 marked this pull request as draft September 24, 2022 09:02
@lucifer1004
Copy link
Member Author

The current version can already emit some basic Julia code.

@certik
Copy link
Contributor

certik commented Sep 24, 2022

Excellent!

lucifer1004 and others added 2 commits September 24, 2022 21:16
Co-authored-by: Ondřej Čertík <ondrej@certik.us>
@lucifer1004
Copy link
Member Author

lfortran --show-julia src/runtime/pure/lfortran_intrinsic_kind.f90
function dkind(x)::Int32
    local r
    r = 8
    return r
end

function kind(x)::Int32
    local r
    r = 4
    return r
end

function lkind(x)::Int32
    local r
    r = 4
    return r
end

function selected_char_kind(r)::Int32
    local res
    res = 1
    return res
end

function selected_int_kind(r)::Int32
    local res
    if r < 10
        res = 4
    else
        res = 8
    end
    return res
end

function selected_real_kind(p)::Int32
    local res
    if p < 7
        res = 4
    else
        res = 8
    end
    return res
end

function skind(x)::Int32
    local r
    r = 4
    return r
end

@lucifer1004
Copy link
Member Author

lucifer1004 commented Sep 24, 2022

@certik How should I test Fortran files that include use ...?

module test
   use lfortran_intrinsic_builtin
   implicit none
contains
end module

Trying to use lfortran --show-asr test.f90 causes

Internal Compiler Error: Unhandled exception
Traceback (most recent call last):
LCompilersException: read_string: String is too short for deserialization.

lfortran --show-ast works normally.

This issue keeps me from testing functions involving intrinsics.

@lucifer1004
Copy link
Member Author

Currently, the following Fortran source

module test
   implicit none
contains

   subroutine a(in, out)
      integer, intent(in) :: in
      integer, intent(out) :: out
      integer :: i

      if (.true.) print *, "in a"

      do i=1,10,3
         if (i > 4) then
            print *, i * in - out + 100 - 23 / 23
         else if (i == 1) then
            print *, i - in, "Hello", 1 + 2, .true..neqv..false.
         else
            print *, 1000
         end if
      end do

      out = in
   end subroutine
end module

program hello
   use test
   implicit none
   integer :: out

   call a(8, out)
end program hello

is translated into

module test

function a(in, out)
    local i
    if true
        println("in a")
    end
    for i in 1:3:10
        if i > 4
            println(i*in - out + 100 - 23/23)
        else
            if i == 1
                println(i - in, " ", "Hello", " ", 1 + 2, " ", true  false)
            else
                println(1000)
            end
        end
    end
    out = in
end

end

function main()
    local out
    a(8, out)
end

main()

However, the module is not handled correctly.

@lucifer1004
Copy link
Member Author

Seems that Use_t only exists in AST but not ASR. Then how should I handle use statements correctly?

@certik
Copy link
Contributor

certik commented Sep 24, 2022

The first error is probably due to a missing mod file for the module you are trying to use. We have to improve the error message.

Regarding your second question, this is handled by the ExternalSymbol ASR node. So when you encounter ExternalSymbol, simply create an equivalent of "use", the node has information which module it is in and what symbol. You can choose to "import" the symbol anywhere it makes sense in Julia, either in the scope where the ExternalSymbol is, or higher up, whatever makes the most sense.

@lucifer1004
Copy link
Member Author

The first error is probably due to a missing mod file for the module you are trying to use. We have to improve the error

I have put the lfortran_intrinsic_builtin.mod in the same folder, but the error still occurs.

@lucifer1004
Copy link
Member Author

@certik Please have a look at the latest commit. I have added a few tests, but when I tried to run them via run_tests.py, none is executed.

@lucifer1004 lucifer1004 marked this pull request as ready for review September 27, 2022 00:06
@lucifer1004
Copy link
Member Author

I have successfully generated the references. There seems to be some issue with the test framework. It currently fuzzy matches filename instead of directly using the options specified in tests.toml. I modified them a bit so that Julia tests could be run. But I think further modifications are needed since the current setting leads to unnecessary or unintended test runs, i.e., when one specifies some tests with the -t option, not only those specified are run, but also some others.

@lucifer1004
Copy link
Member Author

The failed test does not seem to relate to this PR?

@certik
Copy link
Contributor

certik commented Sep 27, 2022

The failing test:

[NbConvertApp] Converting notebook Demo2.ipynb to notebook
std::exception: [json.exception.parse_error.101] parse error at line 1, column 1: syntax error while parsing value - unexpected end of input; expected '[', '{', or a literal
[NbConvertApp] ERROR | Error occurred while starting new kernel client for kernel 9128287f-4aed-4e76-a646-ba9360d8a44f: Kernel died before replying to kernel_info
Traceback (most recent call last):

Is due to #584, but I have not figured out why it sometimes happens.

@certik
Copy link
Contributor

certik commented Sep 27, 2022

I have not used the -t lately. But your changes look good to me, it seems the tests are run correctly?

If there is still some issue, can you describe exactly what command you run, what the expected output is and what the actual output is?

if specific_test:
# some fuzzy comparison to get all seemingly fitting tests tested
specific = [test for test in test_data["test"]
if specific_test in test["filename"]]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, we should use specific_test in test only instead of using specific_test in test["filename"] which might include non-related tests. For example, a test named array_col.f90 has c in it and will thus be included when we use -t c. I did not modify this because there might be some old tests relying on this, which are not explicitly specified in tests.toml.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. go ahead and fix it!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might submit a separate issue and PR for this after the Julia backend PR gets merged.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the idea was to be able to find tests that have that substring in the name, as I think that's how py.test works, but I think it's probably better to select tests exactly by name.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Since we already have a tests.toml to define the testsets for each backend, I think it is more suitable to strictly follow it instead of causing unintended test runs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made this to mimick ctest -R since I think this is really useful if you don't know the entire test from the top of your head.

@certik
Copy link
Contributor

certik commented Sep 27, 2022

Anything else to be done here, or is this ready for final review?

@lucifer1004
Copy link
Member Author

I think it is ready for a final review.

@lucifer1004
Copy link
Member Author

I've spent a lot of effort and time on this PR these days, and I think after it's merged, I'll take a breather, and then continue to work on the remaining issues. Some are already known, while more are to be discovered.

Can also be queried by `Base.operator_precedence()`.
Different from C++, the larger the number, the higher the precedence. To follow LFortran's
convention, we need to revert the precedence table.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LFortran does not have a convention for precedence. The ASR is always explicit. The backends are free to use whatever convention they want. So in here you can choose whatever makes the most sense.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reverse the precedence table instead of reversing precedence comparisons since the former is collected while the latter is separated in many places. The problem here is that there are currently no strong tests for operator precedence.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The best approach should be generating many random expressions, using Julia to run the generated code, and comparing the results with Fortran outputs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. If you want, we can meet to discuss this more. In ASR the precedence is encoded in how the tree is structured, so it is correct there. In the backend you can either always put parentheses everywhere, which is correct, but ugly. The precedence approach allows to generate natural looking Julia (in this case) code. The way I tested this in C and C++ backends was to simply translate this file: https://github.com/lfortran/lfortran/blob/38bcb788a7b1a913759af38e3b67d546e937ecec/integration_tests/expr_05.f90 and add it as a test, and visually check that it looks good.

We could improve the test to actually compute the expression and check that the result is good. All the integration tests could in principle be run with the Julia backend via Julia itself.

Copy link
Contributor

@certik certik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a very good start and good to merge as is.

Is it ok if I squash all the 38 commits into just one?

@lucifer1004
Copy link
Member Author

I think this is a very good start and good to merge as is.

Is it ok if I squash all the 38 commits into just one?

Up to you!

@certik certik enabled auto-merge (squash) September 27, 2022 03:06
@certik certik disabled auto-merge September 27, 2022 03:08
@lucifer1004
Copy link
Member Author

Maybe we should rerun the CI?

@certik certik merged commit 38bcb78 into lfortran:main Sep 27, 2022
@certik
Copy link
Contributor

certik commented Sep 27, 2022

Sorry, I forgot to override the CI.

@certik
Copy link
Contributor

certik commented Sep 27, 2022

Thanks again @lucifer1004 for all your hard work on this. I think the Julia community will be very excited about this.

@chriselrod, you might be interested in this. Once this is more developed, we could use it to feed code to your LoopVectorization project.

@certik certik mentioned this pull request Sep 27, 2022
konradha pushed a commit to konradha/lfortran that referenced this pull request Sep 27, 2022
This implements an initial ASR->Julia backend.
@chriselrod
Copy link

Thanks again @lucifer1004 for all your hard work on this. I think the Julia community will be very excited about this.

@chriselrod, you might be interested in this. Once this is more developed, we could use it to feed code to your LoopVectorization project.

This so cool.
FWIW, the rewrite is still progressing, but it'll probably take another year at least before it is usable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants