Automatically calculated value member of expr nodes in asr #319

Oshanath · 2022-04-05T06:47:49Z

Created the function make_ImplicitCast_t_value() in ASRUtils that will create an implicit cast node and calculate the value attribute if possible. If not possible, function is identical to make_ImplicitCast_t().
Replaced the make_ImplicitCast_t() calls in python_ast_to_asr.cpp with make_ImplicitCast_t_value().
Made the function make_ImplicitCast_t_value() non-inline since it will be large.
IntegerToReal cast fails test cases in integration_tests/test_builtin_pow.py. When both arguments of the pow() are integer and right arg is negative, IntegerToReal cast node gets created with a constantReal_t as the argument.
IntegerToInteger cast fails test cases in integration_tests/test_types_01.py.
So both of the above casts along with IntegerToLogical remain unimplemented for now.
Could not write any tests. Help needed for that. The purpose of this PR is for all implicitCast_t nodes to have a value in the 'value' attribute if it can be computed. But by the time integration tests are run, both compile time and runtime evaluations are complete. So there's no way to distinguish between the two without print statements. Any help regarding this would be appreciated.

Fixes #294

Oshanath · 2022-04-05T06:49:40Z

@certik and @czgdp1807 I mistakenly deleted the fork I made from my github account. That closed the previous pull request. This is identical to the previous one. Sorry about the mess.

certik · 2022-04-06T00:13:11Z

src/libasr/asr_utils.cpp

@@ -713,6 +714,68 @@ ASR::asr_t* symbol_resolve_external_generic_procedure_without_eval(
    }
 }

+ASR::asr_t* make_ImplicitCast_t_value(Allocator &al, const Location &a_loc, ASR::expr_t* a_arg, ASR::cast_kindType a_kind, ASR::ttype_t* a_type, ASR::expr_t* value) {


Wouldn't a better API be to remove the value argument, and the idea being that you call this function if you want to compute the value automatically? But if you know value ahead of time, then you simply call the make_ImplicitCast_t directly.

The reason I included the value argument is because now we can call only the make_ImplicitCast_t_value() and if there is a value, it can be included, and if the value is not provided, it will be nullptr by default, and the function will calculate the value of value.

So now we can stop using make_ImplicitCast_t() completely and just use make_ImplicitCast_t_value() for all purposes, since it will again call make_ImplicitCast_t() inside it.

If this is bad I will change it like you said.

@certik I made a commit to the second suggestion you made. I will wait until you confirm this one again to make the other necessary changes.

I see. It seems that you would almost always call this with "value=nullptr" as the argument, wouldn't you? In that case, I would just remove the argument altogether.

This is an example where the value is passed when it is called. If two functions with and without the value MUST be present, I will make it so. I'm saying we can make do with using only one function for both cases.

So, there are two cases,

value arg is not nullptr - In this case you are just creating the ImplicitCast node and returning it.

value arg is nullptr - In this case you are doing the computation to assign something to value.

The second case makes sense. Is there any example when value is not nullptr. Because if its not nullptr then its already computed for ImplicitCast node and if that's the case then why should we even call this function?

So, the bottom line is if you know that value is not nullptr then directly call, make_ImplicitCast_t and at places where you are passing nullptr as value argument of your function just don't do that and instead update the API here to not accept any value parameter. That should do the job.

I see the design now and have corrected it in the new commit. Thank you @certik and @czgdp1807 for explaining.

src/libasr/asr_utils.cpp

certik · 2022-04-06T00:15:02Z

I left some comments above, otherwise this looks good.

Oshanath · 2022-04-07T14:23:03Z

@certik There is an issue.
TEST: doconcurrentloop_01.py * ast ✓ * asr The JSON metadata differs against reference results Reference JSON: tests/reference/asr-doconcurrentloop_01-3fdc189.json Output JSON: tests/output/asr-doconcurrentloop_01-3fdc189.json

This happens because the reference ASR has no value in the value attribute, but now it does. I checked the differences. Now the ASR has constants embedded (value), which the reference don't.
I think the test cases should be updated. What is your advice?

(There is also the question why the Windows CI passed. It should fail. I will look into that too.)

certik · 2022-04-08T05:04:29Z

Yes, update the tests using ./run_tests.py -u.

czgdp1807 · 2022-04-09T10:10:53Z

I think reference tests should be updated again after merging master into your branch. Ignore conflicts though, because the conflicts in reference tests will be overwritten (and hence resolved automatically) once you run, python run_tests.py -u.

Oshanath · 2022-04-11T18:59:51Z

@certik @czgdp1807 Even though I always merge with the main branch before pushing, I always end up with merge requests. It wasn't there when I pushed it though. It occured later. What should I do?

certik · 2022-04-12T04:04:02Z

Don't worry about it. I am sorry I was late to review --- I'll try to review tomorrow and the conflicts are just tests which can be fixed easily by "./run_tests.py -u", I can do it too before merging.

src/libasr/asr_utils.cpp

src/libasr/asr_utils.h

certik · 2022-04-13T03:15:25Z

I finally got to review this. This is very good, I think the change is correct. I left some minor formatting changes and other simplifications. After you fix those (you can just click on the buttons to commit the changes and just follow the same style).

I noticed some of the compile time computations are commented out --- I assume some tests fail with them? If so, we should fix it later.

Overall it looks good. Please ping me once you fix it. I'll do a final review and we can merge.

certik · 2022-04-13T21:59:38Z

src/libasr/asr_utils.cpp

+            ASR::ConstantComplex_t* value_complex = ASR::down_cast<ASR::ConstantComplex_t>(ASRUtils::expr_value(a_arg));
+            double real = value_complex->m_re;
+            value = ASR::down_cast<ASR::expr_t>(ASR::make_ConstantReal_t(al, a_loc, real, a_type));
+        }


If you can fix the formatting of these just like I suggested above, that would be great. I think it's ready after that.

namannimmo10 · 2022-04-14T16:01:57Z

src/libasr/asr_utils.cpp

+
+    }
+
+    return ASR::make_ImplicitCast_t(al, a_loc, a_arg, a_kind, a_type, value);


ImplicitCast is now Cast in ASR -- see #362.

Thirumalai-Shaktivel · 2022-04-16T10:32:38Z

Hi @Oshanath, I resolved the conflicts and formatting parts. Please review once and make sure the changes are correct as you wanted them to be in this PR.
If everything looks good, ping certik to review the implementation.

Oshanath · 2022-04-16T19:56:20Z

Hi @Oshanath, I resolved the conflicts and formatting parts. Please review once and make sure the changes are correct as you wanted them to be in this PR. If everything looks good, ping certik to review the implementation.

@Thirumalai-Shaktivel Thank you so much for this. I read through everything and it is exactly how it should be. @certik

certik · 2022-04-17T03:25:47Z

I think the patch now looks good. @Oshanath will you be able to polish the history? I would do one commit that implements the new function, another commit that updates the rest of the code. And one commit that updates the tests.

Let's keep @Thirumalai-Shaktivel's commit, to preserve credits.

@Oshanath if you don't know how to do it, please let me know and I can help.

Oshanath · 2022-04-17T19:11:06Z

I think the patch now looks good. @Oshanath will you be able to polish the history? I would do one commit that implements the new function, another commit that updates the rest of the code. And one commit that updates the tests.

Let's keep @Thirumalai-Shaktivel's commit, to preserve credits.

@Oshanath if you don't know how to do it, please let me know and I can help.

Could you please direct me to some learning resource on cleaning the git history?

certik · 2022-04-19T04:22:08Z

The keyword is "git interactive rebase", I found, e.g.: https://www.sitepoint.com/git-interactive-rebase-guide/

The alternative is to use git reset master and git add -p and git commit and create a new set of patches.

Important: make sure you make a backup of your branch before you do any of this. If you make a mistake, just use the backup and start over.

If you get stuck, just ask for help, we can help.

Oshanath · 2022-04-19T09:56:18Z

@certik I will create a backup. If something goes wrong what I should do is close this PR, create another PR from the backup. Is that correct?

certik · 2022-04-19T11:52:47Z

No, just keep this PR. Fix up your local branch, make sure it looks good (git log --stat) and then force push into this PR.

namannimmo10 · 2022-05-02T17:22:13Z

@Oshanath -- let us know if you need any help.

certik · 2022-05-09T18:07:25Z

This looks good to merge, we just have to rebase.

This function computes the compile time value automatically if possible. Formatted-by: Thirumalai Shaktivel <thirumalaishaktivel@gmail.com>

Now compile time values are always correctly computed in cast nodes.

certik

I think this is good to go in. We have thanked @Thirumalai-Shaktivel in the commit, as it was hard to preserve the commits due to the many changes needed.

czgdp1807 changed the title ~~first commit~~ Automatically calculated value member of expr nodes in asr Apr 5, 2022

certik reviewed Apr 6, 2022

View reviewed changes

src/libasr/asr_utils.cpp Outdated Show resolved Hide resolved