|Anonymous | Login | Signup for a new account||2017-09-25 19:06 CEST|
|Main | My View | View Issues | Change Log | Roadmap|
|View Issue Details|
|ID||Project||Category||View Status||Date Submitted||Last Update|
|0005346||OCaml||~DO NOT USE (was: OCaml general)||public||2011-08-21 09:20||2016-12-07 13:57|
|Status||closed||Resolution||no change required|
|Target Version||later||Fixed in Version|
|Summary||0005346: Improve Cmm code generator for "for" loops.|
|Description||The code generated by cmmgen for Ufor loops doesn't optimize in case of constant low and high expressions. This is fixed by a special case where the initial comparison can be skipped in case of constant low and high subexpressions.|
In addition, this patch also reorders the generated code by moving the initial comparison out of the catch block in the general case. This may enable certain optimizations in later phases.
|Attached Files||0001-Improve-Cmm-code-generator-for-for-loops.patch [^] (3,146 bytes) 2011-08-21 09:20 [Show Content]|
First of all, thanks for the patch. Disclaimer: I'm in no way an expert of Cmm. Here are some preliminary remarks, though.
- Your patch adds complexity for what seem to be little benefit. The test is an initial one, it's not in the loop. You're just skipping a couple instructions. Is it worth the extra complexity of your approach? Can you justify better why we should make that case harder to understand in cmmgen?
- There are no comments in your patch, which resonates with the point above. Moreover, the current version has an id_prev identifier defined. While I fail to see why it's there in the first place (your version of the test seems both shorter and correct), I'd appreciate if you could share your thoughts on this.
Thinking about performance, even in the case of nested loops, say
for i = 0 to n; do
for j = 0 to i; do
then the extra test that might have been run n+1 times wouldn't be skipped since j is not a constant expression. This seems to me like a very ad-hoc optimization that adds extra complexity for close to zero benefit. I'd like to be proved wrong, though :) so please let me know why you think this optimization has to be included.
A benchmark on some real-world code may help me see better why you want this.
|Not urgent enough for 4.01.|
|No activity on this PR in the last 4 years. Should we keep it alive or suspend it?|
We have some vague plans for local CPS constructs in flambda in the future; I wouldn't be surprised, having expressed these looping constructions like that, if things such as this PR just fall out in the wash.
So I would vote for closing this.
|CPS-flambda should sort this out. Closing|
|2011-08-21 09:20||meurer||New Issue|
|2011-08-21 09:20||meurer||File Added: 0001-Improve-Cmm-code-generator-for-for-loops.patch|
|2011-12-16 18:02||protz||Note Added: 0006330|
|2011-12-21 14:23||gasche||Status||new => feedback|
|2012-09-06 16:43||doligez||Target Version||=> 4.00.1+dev|
|2012-09-20 16:27||doligez||Target Version||4.00.1+dev => 4.01.0+dev|
|2013-07-19 11:43||xleroy||Note Added: 0009812|
|2013-07-19 11:43||xleroy||Target Version||4.01.0+dev => 4.01.1+dev|
|2013-10-09 14:15||doligez||Tag Attached: patch|
|2014-05-25 20:20||doligez||Target Version||4.01.1+dev => 4.02.0+dev|
|2014-07-16 20:50||doligez||Target Version||4.02.0+dev => 4.02.1+dev|
|2014-09-04 00:25||doligez||Target Version||4.02.1+dev => undecided|
|2014-09-22 22:13||doligez||Target Version||undecided => 4.02.2+dev / +rc1|
|2015-01-15 00:32||doligez||Target Version||4.02.2+dev / +rc1 => 4.03.0+dev / +beta1|
|2015-11-23 15:47||xleroy||Note Added: 0014803|
|2015-11-23 18:10||shinwell||Note Added: 0014810|
|2015-11-27 18:25||frisch||Target Version||4.03.0+dev / +beta1 => later|
|2016-12-07 13:56||shinwell||Note Added: 0016701|
|2016-12-07 13:57||shinwell||Status||feedback => closed|
|2016-12-07 13:57||shinwell||Resolution||open => no change required|
|2017-02-23 16:36||doligez||Category||OCaml general => -OCaml general|
|2017-03-03 17:55||doligez||Category||-OCaml general => -(deprecated) general|
|2017-03-03 18:01||doligez||Category||-(deprecated) general => ~deprecated (was: OCaml general)|
|2017-03-06 17:04||doligez||Category||~deprecated (was: OCaml general) => ~DO NOT USE (was: OCaml general)|
|Copyright © 2000 - 2011 MantisBT Group|