Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle external interface in convert_call_args #2800

Merged
merged 3 commits into from
Nov 5, 2023

Conversation

Pranavchiku
Copy link
Contributor

@Pranavchiku Pranavchiku commented Nov 4, 2023

Fixes #1977. Fixes #2748. Fixes #2427

@Pranavchiku Pranavchiku added the SciPy issues related to enable LFortran to compile the entire FORTRAN codebase in SciPy label Nov 4, 2023
ret i32 0
}

declare void @_lpython_set_argv(i32, i8**)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Generated LLVM looks good to me.

@Pranavchiku
Copy link
Contributor Author

Pranavchiku commented Nov 4, 2023

This case do not work yet:

subroutine dqc25f()
   external f
   call dqk15w(f)
end

subroutine dqk15w(f)
   interface
      function f(x) result (r)
         real r, x
      end function
   end interface
   real r
   r = f(2.0)
   print *, r
end subroutine

program main
   call dqc25f()
end program

Because, interface of f is updated in subroutine dqk15w , right now we only append to changed_external_function_symbol if it is external interface. It is easy fix.


define void @b() {
.entry:
call void @a(float (float*)* @f.1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Here is it calling f.1, meaning that it won't be able to find the definition of this at link time, correct?

@certik
Copy link
Contributor

certik commented Nov 4, 2023

Can you try to compile and link an example? I think it won't work, due to a missing f.1 symbol.

@Pranavchiku
Copy link
Contributor Author

This case works correctly: #2748 (comment)

Here it has concrete definition of f

@certik
Copy link
Contributor

certik commented Nov 4, 2023

How exactly does the f.1 symbol get resolved? We need to understand that.

@certik
Copy link
Contributor

certik commented Nov 5, 2023

@Pranavchiku to move forward:

  • can you add an integration test that fails in master but now works? Then we can merge this.
  • As a separate investigation, we need to understand what is happening with the f.1.

@Pranavchiku
Copy link
Contributor Author

I added the test, this PR can have a review.

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 looks good now, thanks!

@certik certik merged commit 32a8d90 into lfortran:main Nov 5, 2023
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
SciPy issues related to enable LFortran to compile the entire FORTRAN codebase in SciPy
Projects
None yet
2 participants