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

Fix ASR verify pass error while using Interactive #4086

Merged
merged 4 commits into from
Jun 7, 2024

Conversation

Vipul-Cariappa
Copy link
Contributor

Reference lcompilers/lpython#2706

This PR is required by #4006

CHECK(r.ok);
CHECK(r.result.type == FortranEvaluator::EvalResult::integer4);
CHECK(r.result.i32 == 1);
}
Copy link
Member

@Shaikh-Ubaid Shaikh-Ubaid May 25, 2024

Choose a reason for hiding this comment

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

Since you are adding this test, maybe try to use/test a variety of types like integer8, real4, real8, logical (along with integer4)?

Copy link
Member

Choose a reason for hiding this comment

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

This just makes the test more robust and makes it easier to catch bugs earlier than later in development.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The main purpose of the tests is to detect function dependencies related bugs. In all the tests I have written, there are 2 functions one dependent on the other.
I have added another test that deals with real values. Once we merge #4006 we will have another test dealing with logical values. I guess that should be sufficient tests.

I can write more tests if you think this is insufficient.

Copy link
Member

Choose a reason for hiding this comment

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

The main purpose of the tests is to detect function dependencies related bugs. In all the tests I have written, there are 2 functions one dependent on the other.

It makes sense. It is fine for now.

@Shaikh-Ubaid
Copy link
Member

Please mark as "Ready for review" when ready.

@Vipul-Cariappa Vipul-Cariappa marked this pull request as ready for review May 25, 2024 10:32
@Shaikh-Ubaid
Copy link
Member

Shaikh-Ubaid commented May 25, 2024

D:\a\lfortran\lfortran\lfortran-0.35.0-292-gdda8554e2\src\lfortran\tests\test_llvm.cpp(1175): ERROR: test case THREW exception: lookup() failed to find the symbol '__lfortran_evaluate_4', error: Failed to materialize symbols: { (Main, { __lfortran_evaluate_4 }) }

There seems a failure on windows. We need the CI to be green to merge this.

PS: For now, I have rerun it. Let's see if it passes.

@Shaikh-Ubaid
Copy link
Member

D:\a\lfortran\lfortran\lfortran-0.35.0-292-gdda8554e2\src\lfortran\tests\test_llvm.cpp(1175): ERROR: test case THREW exception: lookup() failed to find the symbol '__lfortran_evaluate_4', error: Failed to materialize symbols: { (Main, { __lfortran_evaluate_4 }) }

There seems a failure on windows. We need the CI to be green to merge this.

PS: For now, I have rerun it. Let's see if it passes.

The failure on windows seems real. We need to fix it.

@Shaikh-Ubaid Shaikh-Ubaid marked this pull request as draft May 25, 2024 11:34
@Vipul-Cariappa
Copy link
Contributor Author

For reference:
Windows error output:

$ ./src/lfortran/tests/Release/test_lfortran.exe
[doctest] doctest version is "2.4.8"
[doctest] run with "--help" for options
0 0 0 0 0 0 0 0 0 0
JIT session error: Failed to materialize symbols: { (Main, { sub }) }
JIT session error: Failed to materialize symbols: { (Main, { __real@80000000 }) }
===============================================================================
C:\Users\vipul\Documents\Workspace\lfortran\src\lfortran\tests\test_llvm.cpp(1175):
TEST CASE:  FortranEvaluator asr verify 3

C:\Users\vipul\Documents\Workspace\lfortran\src\lfortran\tests\test_llvm.cpp(1175): ERROR: test case THREW exception: lookup() failed to find the symbol '__lfortran_evaluate_4', error: Failed to materialize symbols: { (Main, { __lfortran_evaluate_4 }) }

===============================================================================
[doctest] test cases:  87 |  86 passed | 1 failed | 0 skipped
[doctest] assertions: 607 | 607 passed | 0 failed |
[doctest] Status: FAILURE!

@certik
Copy link
Contributor

certik commented May 26, 2024

Yes, Windows always gave me a lot of issues with regards to interactivity.

@Vipul-Cariappa Vipul-Cariappa marked this pull request as ready for review June 6, 2024 06:17
@Vipul-Cariappa
Copy link
Contributor Author

I have managed to fix the errors on Windows by modifying KaleidoscopeJIT.h according to the LLVM's KaleidoscopeJIT.h from the 16.0.6 version.

Please have a look at it and let me know if it requires any more changes.

Copy link
Member

@Shaikh-Ubaid Shaikh-Ubaid left a comment

Choose a reason for hiding this comment

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

It looks good to me! Thanks!

@Vipul-Cariappa updated the KaleidoscopeJIT referring the LLVM 16 documentation. The changes look good to me. These affect only the JIT mode and the regular mode should be fine.

Just to be safe, @certik can you have a quick glance through it?

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 that looks good to me!

Does interactive mode still work with older LLVM versions?

@Vipul-Cariappa
Copy link
Contributor Author

Yes. Interactive mode works in all the LLVM versions we test with.

@certik certik merged commit d50cfed into lfortran:main Jun 7, 2024
23 checks passed
@certik
Copy link
Contributor

certik commented Jun 7, 2024

I merged it, but unfortunately CI fails after merging: https://github.com/lfortran/lfortran/actions/runs/9410908077/job/25923326818

The following tests FAILED:
Errors while running CTest
	314 - intrinsics_74 (Failed)

@Vipul-Cariappa can you please investigate why it might be failing?

If you can fix it quickly, then let's do it. Otherwise I'll have to revert this, not to block other work with the CI failure.

@certik
Copy link
Contributor

certik commented Jun 7, 2024

Looking at the failing test:

program intrinsics_74
    implicit none

    real :: x(1, 2)
    x = 0
    call srand(0)
    call random_number(x(1, 2))
    print *, x

    if (abs(x(1, 1) - 0.0) > 1e-5) error stop
    if (abs(x(1, 2) - 0.0) < 1e-5) error stop
end program

I am not sure if it is related to this PR or not. CC @Shaikh-Ubaid

@certik
Copy link
Contributor

certik commented Jun 7, 2024

Ok, here is the failure:

314/992 Test #314: intrinsics_74 ............................***Failed    0.01 sec
0.00000000e+00 3.84403393e-06 
�
ERROR STOP

So that's a bad test, unrelated to this PR.

@Vipul-Cariappa
Copy link
Contributor Author

Good to know, my changes did not cause the problem.

@certik
Copy link
Contributor

certik commented Jun 7, 2024

I rerun the CI, it works. So this PR is fine.

The actual bug is #4167, unrelated to this PR.

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.

None yet

3 participants