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.

Sign up for GitHub

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

Jump to bottom

feature[next]: Remove int type from FOAST, PAST, ITIR #1255

Merged
merged 39 commits into from
Jun 14, 2023
Merged

feature[next]: Remove int type from FOAST, PAST, ITIR #1255

merged 39 commits into from
Jun 14, 2023

Conversation

tehrengruber
Copy link
Contributor

@tehrengruber tehrengruber commented May 22, 2023 •
edited
Loading

This PR removes all occurrences of the int, float type and replaces them by the fixed size types int32, int64, float32, float64. In particular int was problematic as an undetermined size lead to problems in type inference giving strange errors akin of Can not satisfy constraint on primitive types: int ≡ int32.

Blocked by #1258 (already merged in this PR).

@tehrengruber tehrengruber changed the title Remove int type (without size) from FOAST, PAST, ITIR feature[next]: Remove int type (without size) from FOAST, PAST, ITIR May 22, 2023
@tehrengruber tehrengruber changed the title feature[next]: Remove int type (without size) from FOAST, PAST, ITIR feature[next]: Remove int type from FOAST, PAST, ITIR May 22, 2023
@tehrengruber tehrengruber mentioned this pull request May 30, 2023
Closed
@egparedes egparedes requested a review from havogt May 30, 2023 12:49
@tehrengruber tehrengruber mentioned this pull request May 31, 2023
Merged
havogt
havogt requested changes Jun 6, 2023
View reviewed changes
Copy link
Contributor

@havogt havogt left a comment

Choose a reason for hiding this comment

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

Main concern is the duplication of value to literal translation and hiding it in ir_makers.

src/gt4py/next/iterator/ir_makers.py Outdated Show resolved Hide resolved
src/gt4py/next/iterator/ir_makers.py Outdated Show resolved Hide resolved
src/gt4py/next/iterator/pretty_parser.py Outdated Show resolved Hide resolved
src/gt4py/next/iterator/ir_makers.py Outdated Show resolved Hide resolved
src/gt4py/next/iterator/tracing.py Outdated Show resolved Hide resolved
tests/next_tests/integration_tests/feature_tests/cases.py Outdated Show resolved Hide resolved
tests/next_tests/integration_tests/feature_tests/ffront_tests/test_execution.py Outdated Show resolved Hide resolved
tests/next_tests/integration_tests/feature_tests/ffront_tests/test_execution.py Outdated Show resolved Hide resolved
tests/next_tests/integration_tests/feature_tests/ffront_tests/test_gt4py_builtins.py Outdated Show resolved Hide resolved
tests/next_tests/integration_tests/feature_tests/ffront_tests/test_scalar_if.py Outdated Show resolved Hide resolved
@tehrengruber tehrengruber requested a review from havogt June 13, 2023 01:20
@tehrengruber
Copy link
Contributor Author

@havogt Mostly as discussed. I have introduced a new function literal_from_value in ir_makers and left literal as is. Since literal_from_value does not itself introduce any new logic it made sense to me to leave it in ir_makers which contains other functions with similar tasks (i.e. ensure_expr).

havogt
havogt approved these changes Jun 14, 2023
View reviewed changes
Copy link
Contributor

@havogt havogt left a comment

Choose a reason for hiding this comment

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

just a question, because I don't remember.

src/gt4py/next/ffront/fbuiltins.py
@@ -29,6 +30,12 @@
TYPE_BUILTINS = [Field, Dimension, int32, int64, float32, float64] + PYTHON_TYPE_BUILTINS
TYPE_BUILTIN_NAMES = [t.__name__ for t in TYPE_BUILTINS]

# Be aware: Type aliases are not fully supported in the frontend yet, e.g. `IndexType(1)` will not
# work.
IndexType: TypeAlias = int32
Copy link
Contributor

Choose a reason for hiding this comment

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

What was the problem why we decided to add an index type? I only realize now that this is a potential problem for bindings, if the driver code has an opinion on the size of IndexType. But let's keep it for now...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You suggested it as it is occurring in the tests multiple times (e.g. in IndexFields). I think the bindings will still accept int64 for NeighborTableOffsetProvider.

Copy link
Contributor

Choose a reason for hiding this comment

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

I suggested the alias but why did we introduce this constraint https://github.com/GridTools/gt4py/pull/1255/files#diff-1c4cbeb95e161c4e1ca0327817608ba3a8b48569e756847b2093bc43606863d6R685

@tehrengruber tehrengruber merged commit 0b668fb into GridTools:main Jun 14, 2023
This was referenced Jun 16, 2023
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@havogt havogt havogt approved these changes

@egparedes egparedes Awaiting requested review from egparedes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tehrengruber @havogt