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

Support logical values when using interactive #4006

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Vipul-Cariappa
Copy link
Contributor

@Vipul-Cariappa Vipul-Cariappa commented May 14, 2024

reference: #4005

case (LCompilers::FortranEvaluator::EvalResult::boolean) : {
if (verbose) std::cout << "Return type: logical" << std::endl;
if (verbose) section("Result:");
std::cout << (r.b ? "True" : "False") << std::endl;
Copy link
Contributor

Choose a reason for hiding this comment

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

In Fortran you want to print this as:

Suggested change
std::cout << (r.b ? "True" : "False") << std::endl;
std::cout << (r.b ? ".true." : ".false.") << std::endl;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am printing it based on what print function does

>>> print *, .true.
True

Do you want me to keep this existing implementation or change it to your suggestion?

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. Leave True then for now. We can change both later. In fact I think GFortran just prints T/F. I prefer True or .true., but for compatibility we'll at least have to add a compiler option to print just T/F. But that's a separate issue.

@@ -102,6 +102,8 @@ std::string LLVMModule::get_return_type(const std::string &fn_name)
return "real4";
} else if (type->isDoubleTy()) {
return "real8";
} else if (type->isIntegerTy(1)) {
return "logical";
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 we use 4 bytes integer as boolean, not 1 byte.

Copy link
Contributor

Choose a reason for hiding this comment

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

Consequently, in LLVM we cannot fully determine the high level ASR type, since things are lowered (information got lost if we started with logical or integer).

So I think this function has to do its best with the information that it has, and somewhere else we need to use the ASR itself to determine what the return type should be and if it is compatible with the lowered type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried changing type->isIntegerTy(1) to type->isIntegerTy(4) the tests failed (New tests I added).

The isIntegerTy takes in Bitwidth, I don't think it is bytes.

@Vipul-Cariappa
Copy link
Contributor Author

cc @Shaikh-Ubaid

CHECK(r.result.type == FortranEvaluator::EvalResult::boolean);
CHECK(!r.result.b);
}

Copy link
Member

Choose a reason for hiding this comment

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

Let's add two more tests in this:

  • i = boolean operation on two boolean values
  • i = function_that_returns_boolean_value()

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.

Let's improve the tests. The rest of the things look good to me. Great work!

@certik certik marked this pull request as draft May 18, 2024 23:52
@certik
Copy link
Contributor

certik commented May 18, 2024

@Vipul-Cariappa once it is ready, go ahead and click on "Ready for review".

@Vipul-Cariappa
Copy link
Contributor Author

Tests fail. The reason is:

❯ lfortran
Interactive Fortran. Experimental prototype, not ready for end users.
LFortran version: 0.0.0
  * Use Ctrl-D to exit
  * Use Enter to submit
  * Use Alt-Enter or Ctrl-N to make a new line
    - Editing (Keys: Left, Right, Home, End, Backspace, Delete)
    - History (Keys: Up, Down)
>>> function is_even(n) result(result)                                                                                                             ┐
...   integer, intent(in) :: n                                                                                                                     │
...   logical :: result                                                                                                                            │
...   result = mod(n, 2) == 0                                                                                                                      │
... end function is_even                                                                                                                     5,21  ┘
>>>                                                                                                                                          1,1   ]
ASR verify pass error: Function is_even doesn't depend on _lcompilers_optimization_mod_i32 but is found in its dependency list.
 --> input:1:1 - 2:127
  |
1 |    
  |    ...
...
  |
2 |    
  | ...^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ failed here


Note: Please report unclear, confusing or incorrect messages as bugs at
https://github.com/lfortran/lfortran/issues.

lcompilers/lpython#2706 tries to resolves this.

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