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

feature[next]: Add power unrolling functionality and respective unit tests. #1409

Merged
merged 7 commits into from
Jan 17, 2024

Conversation

SF-N
Copy link
Contributor

@SF-N SF-N commented Jan 5, 2024

PR Title: Add power unrolling functionality and respective unit tests.

<type>:
    - powers of SymRefs and FunCalls until order 5 are unrolled per default
    - powers with higher exponent are still computed by the built-in functions.
    - test: add respective tests

<scope>: next 

@SF-N SF-N marked this pull request as draft January 5, 2024 10:05
@SF-N SF-N marked this pull request as ready for review January 5, 2024 10:07
@tehrengruber tehrengruber self-requested a review January 5, 2024 10:16
if check_node(new_node):
assert len(new_node.args) == 2
# Check if unroll should be performed or if exponent is too large
if int(new_node.args[1].value) > self.max_unroll:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if int(new_node.args[1].value) > self.max_unroll:
base, exponent = new_node.args[0], int(new_node.args[1].value)
if exponent > self.max_unroll:

and so on

@tehrengruber tehrengruber self-requested a review January 8, 2024 11:27
@tehrengruber tehrengruber changed the title Add power unrolling functionality and respective unit tests. feature[next]: Add power unrolling functionality and respective unit tests. Jan 9, 2024
def _is_power_call(
node: ir.FunCall,
) -> bool:
return (
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return (
"""Matches expressions of the form `power(base, integral_literal)`."""
return (

)


def _compute_integer_power_of_two(exp):
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def _compute_integer_power_of_two(exp):
def _compute_integer_power_of_two(exp: itir.Expr) -> int:

Copy link
Contributor

Choose a reason for hiding this comment

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

My bad:

Suggested change
def _compute_integer_power_of_two(exp):
def _compute_integer_power_of_two(exp: int) -> int:

max_unroll: int

@classmethod
def apply(cls, node: ir.Node, max_unroll=5) -> ir.Node:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def apply(cls, node: ir.Node, max_unroll=5) -> ir.Node:
def apply(cls, node: ir.Node, max_unroll: int=5) -> ir.Node:


ret = im.multiplies_(ret, f"power_{2 ** pow_cur}")

# In case of FunCall: build nested expression to avoid multiple evaluations of base
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# In case of FunCall: build nested expression to avoid multiple evaluations of base
# nest target expression to avoid multiple redundant evaluations

For clarification: It is not only that we don't want to reevaluate the base expression, but also the various power compontents themselves.


# Simplify expression in case of SymRef by resolving let statements
if isinstance(base, ir.SymRef):
return InlineLambdas.apply(ret)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return InlineLambdas.apply(ret)
return InlineLambdas.apply(ret, opcount_preserving=True)

The proper way would be to somehow tell the inliner to not ignore anything of the base, but we don't have a mechanism for this and the alternatives are less readable. This should be fine.



def test_power_unrolling_zero():
pytest.xfail("Not implemented.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
pytest.xfail("Not implemented.")
pytest.xfail("Not implemented as we don't have an easy way to determine the type of the one literal (type inference is to expensive).")

@SF-N SF-N merged commit 6e6271c into GridTools:main Jan 17, 2024
19 checks passed
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.

2 participants