Skip to content

[SYSTEMDS-2801] Rewrite rule to remove (par)for-loops over empty sequences.#1165

Closed
pdamme wants to merge 1 commit intoapache:masterfrom
pdamme:RewriteRemoveForLoopEmptySequence
Closed

[SYSTEMDS-2801] Rewrite rule to remove (par)for-loops over empty sequences.#1165
pdamme wants to merge 1 commit intoapache:masterfrom
pdamme:RewriteRemoveForLoopEmptySequence

Conversation

@pdamme
Copy link
Contributor

@pdamme pdamme commented Jan 19, 2021

This commit adds a new StatementBlockRewriteRule removing (par)for-loops which are known to iterate over an empty sequence, and, thus, will not perform a single iteration. This works if the loop's from, to, and increment are constants (possibly DML script arguments, and after constant folding). Furthermore, OptimizerUtils has a new flag determining whether this rule shall be applied. In ProgramRewriter's default rule sequence, this new rule is inserted directly after the removal of unnecessary branches, since they have a lot in common. Merging subsequent blocks is done once afterwards if any of removing unnecessary branches or removing
(par)for-loops shall be applied.

…ence.

This commit adds a new StatementBlockRewriteRule removing (par)for-loops
which are known to iterate over an empty sequence, and, thus, will not
perform a single iteration. This works if the loop's from, to, and
increment are constants (possibly DML script arguments, and after constant
folding). Furthermore, OptimizerUtils has a new flag determining whether
this rule shall be applied. In ProgramRewriter's default rule sequence,
this new rule is inserted directly after the removal of unnecessary
branches, since they have a lot in common. Merging subsequent blocks is
done once afterwards if any of removing unnecessary branches or removing
(par)for-loops shall be applied.
@phaniarnab
Copy link
Contributor

Thanks for adding this rewrite.
One suggestion: In general, adding a test case might be a good idea. You can execute a script (local and spark) with and without the rewrite and match the results.

@pdamme
Copy link
Contributor Author

pdamme commented Jan 19, 2021

Good idea. I will add a test case soon.

@mboehm7
Copy link
Contributor

mboehm7 commented Jan 23, 2021

LGTM - welcome to SystemDS @pdamme and thanks for the patch. During the merge, I fixed a few related for/parfor issues (for correctly handling special values like NaN/Inf/-Inf), fixed the issue of my previous commit, simplified and generalized the rewrite a bit, and added the tests you already discussed. The test simply checks that after this rewrite, we do common subexpression elimination of ops before and after the loop.


print(sum(X));
for(i in seq(NaN,1))
  print("Iteration "+i)
print(sum(X))

@asfgit asfgit closed this in 87e4d50 Jan 23, 2021
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.

3 participants

Comments