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

Storage reduction pass #203

Closed
wants to merge 99 commits into from

Conversation

eddie-c-davis
Copy link
Contributor

@eddie-c-davis eddie-c-davis commented Sep 30, 2020

Description

This PR introduces an optimization pass that reduces three dimensional temporary storages (IJK) to two dimensions (IJ) whenever possible.

Eddie Davis added 30 commits August 12, 2020 15:31
@jdahm jdahm requested a review from egparedes October 30, 2020 17:37
@jdahm
Copy link
Contributor

jdahm commented Nov 20, 2020

Could this be changed to use the mask concept used elsewhere instead of using a new axes type argument? Also, we should add tests to ensure it works with storages allocated with different axis masks.

@eddie-c-davis
Copy link
Contributor Author

Thank you, I will look at using masks.

Copy link
Contributor

@jdahm jdahm left a comment

Choose a reason for hiding this comment

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

Getting there. Needs support for 1D fields and masks (eddie-c-davis#5)

offset = "{:+d}".format(node.offset[ax]) if ax in node.offset else ""
index.append("{ax}{offset}".format(ax=ax, offset=offset))

# Extend index with zeros...
index.extend(["0"] * (len(self.block_info.extent) - len(index)))
Copy link
Contributor

Choose a reason for hiding this comment

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

This should use the filter_mask or NumericTuple.from_mask after merging.

Comment on lines 125 to 126
for i in range(len(extent) - len(axes)):
shape += ", 1"
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

{%- endfor %}
{% set arg_counter = arg_counter + arg_fields|length %}
{% for param in parameters %}
using p_{{ param.name }} = gt::arg<{{ arg_counter + loop.index0 }}, gt::global_parameter<{{ param.dtype }}>>;
using p_{{ param.name }} = gt::arg<{{ arg_counter + loop.index0 }}, gt::global_parameter<backend_t, {{ param.dtype }}>>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is backend_t is needed here? You may have gotten this at one point from one of my PRs, but I'm not sure it's really needed after all.

@@ -160,11 +183,12 @@ auto make_grid(const std::array<gt::uint_t, 3>& compute_domain_shape) {
}


{%- for axes in used_axes %}
{%- if axes.name == "ijk" %}
Copy link
Contributor

Choose a reason for hiding this comment

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

1D field support here.

@eddie-c-davis eddie-c-davis marked this pull request as draft December 15, 2020 15:11
@eddie-c-davis
Copy link
Contributor Author

Replaced with temp-storage-reduce branch so closing.

@eddie-c-davis eddie-c-davis deleted the storage_reduction branch January 25, 2021 18:05
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.

4 participants