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

Fix implied do and array constructor (fixes #1200) #1212

Merged
merged 1 commit into from
Mar 5, 2022

Conversation

zhaochuanfeng
Copy link
Contributor

Fix errors when function statement appears in loop body or loop control of
implied-do loop and the actual parameter is an array construction that
depends on the loop variable, the assignment of the temporary array
generated for the array construction is generated outside the loop,
resulting in the use of an uninitialized loop variable.

Now support the following cases:

  1. Multiple layers of implied-do loop nesting.
  2. The loop body in a single array construction or
    the loop body in an implied-do loop contains multiple implied-do loops.
  3. The implied-do loop is converted from " : ".

@kiranchandramohan
Copy link
Collaborator

The CI is fixed. Please rebase to run CI.

@bryanpkc
Copy link
Collaborator

bryanpkc commented Feb 9, 2022

@zhaochuanfeng I already rebased the branch on master a couple of days ago. Can you rebase again and remove the merge commit?

…ray constructor

Fix errors when function statment appears in loop body or loop control of
implied-do loop and the actual parameter is an array construction that
depends on the loop variable, the assignment of the temporary array
generated for the array construction is generated outside the loop,
resulting in the use of uninitialized loop variable.

Now support the following cases:
1. Multiple layers of implied-do loop nesting.
2. The loop body in a single array construction or
   the loop body in an implied-do loop contains multiple implied-do loops.
3. The implied-do loop is converted from "<expression> : <expression> <opt stride>".
@zhaochuanfeng
Copy link
Contributor Author

@zhaochuanfeng I already rebased the branch on master a couple of days ago. Can you rebase again and remove the merge commit?

Sure, alreay done.

@bryanpkc
Copy link
Collaborator

bryanpkc commented Feb 9, 2022

Thanks. I have confirmed that the branch is rebased on the latest master, and is not changed otherwise.

@zaikunzhang
Copy link

I find another issue concerning array constructors: #1227

Do you think it is related to the bug fixed here? Thank you!

@shivaramaarao shivaramaarao merged commit f7a8942 into flang-compiler:master Mar 5, 2022
@pawosm-arm
Copy link
Collaborator

Hello,
A serious regression was found by the nag test suite. The mw4 test case which operates on just a 2-element array makes way too large allocations after merging this PR. Compare these (mw4-safe was compiled before merging this PR):

$ memusage -m ./mw4-safe
 ok

Memory usage summary: heap total: 104294, heap peak: 104230, stack peak: 1808
         total calls   total memory   failed calls
 malloc|         10         104294              0
realloc|          0              0              0  (nomove:0, dec:0, free:0)
 calloc|          0              0              0
   free|          2             64
mmap(r)|          0              0              0
mmap(w)|          0              0              0
mmap(a)|          0              0              0
 mremap|          0              0              0  (nomove: 0, dec:0)
 munmap|          0              0              0
Histogram for block sizes:
    0-15              5  50% ==================================================
   32-47              2  20% ====================
12288-12303           1  10% ==========
19200-19215           1  10% ==========
   large              1  10% ==========
$ memusage -m ./mw4
 ok

Memory usage summary: heap total: 9257652166, heap peak: 9257632928, stack peak: 2112
         total calls   total memory   failed calls
 malloc|         11     9257652166              0
realloc|          0              0              0  (nomove:0, dec:0, free:0)
 calloc|          0              0              0
   free|          2     9257547904
mmap(r)|          0              0              0
mmap(w)|          0              0              0
mmap(a)|          0              0              0
 mremap|          0              0              0  (nomove: 0, dec:0)
 munmap|          0              0              0
Histogram for block sizes:
    0-15              5  45% ==================================================
   32-47              2  18% ====================
12288-12303           1   9% ==========
19200-19215           1   9% ==========
   large              2  18% ====================

This causes failure in our CI where memory restrictions do apply.
@bryanpkc should I open a new github issue for this?

@pawosm-arm
Copy link
Collaborator

pawosm-arm commented May 28, 2022

@zhaochuanfeng As it turned out, the problem with the code added by upstream PR#1212 is with the call to the move_range_after() function in the semutil2.c file.

The move_range_before() and move_range_after() functions were introduced by Gary Klimowicz a couple of years ago and never actually used, even in the commit that has introduced them. Therefore there is no good example on how this function can be used legally.

Apparently, the way it is used in the code added by PR#1212 is not entirely legal. Moving some of the AST nodes ahead in the STD list without checking for possible dependencies turned out to be problematic. In the mw4 test case it resulted in a huge allocation as the computation of the allocation size was moved after the actual allocation, therefore some random value was used and when it was huge, it resulted in the allocation failure. Note that there is potential for any other failure caused by such careless motion of the AST nodes. Unfortunately, not making this move also results in an incorrect execution, so this call to move_range_after() can not be simply removed.

As this is issue is potentially harmful in some very non-obvious ways, this needs to be addressed.

I have worked out a temporary hack (which is not a final solution). After the move is done, the affected nodes are copied back to their original place, but only in the case when there is anything to copy (begin != end, note that in opposite case the move_range_before() function does not actually move anything) and only when the 'implied do' element count is known at compile time (otherwise, the implied_do7 test case is failing).

--- a/tools/flang1/flang1exe/semutil2.c
+++ b/tools/flang1/flang1exe/semutil2.c
@@ -3360,8 +3360,28 @@ _constructf90(int base_id, int in_indexast, bool in_array, ACL *aclp)
       if (aclp->u2.std_range != NULL && aclp->u2.std_range->start != 0) {
         /* move in stmts generated by loop body and replace dovar */
         int do_std = STD_LAST;
-        move_range_after(aclp->u2.std_range->start, aclp->u2.std_range->mid,
-                         do_std);
+        int beg_std = aclp->u2.std_range->start;
+        int end_std = aclp->u2.std_range->mid;
+        int prev_std = STD_PREV(beg_std);
+        int next_std = STD_NEXT(end_std);
+
+        move_range_after(beg_std, end_std, do_std);
+        if ((beg_std != end_std) && CONVAL2G(A_SPTRG(A_ALIASG(doinfo->count)))) {
+          for (int std = beg_std; std != STD_NEXT(end_std); std = STD_NEXT(std)) {
+            int new_ast = mk_duplicate_ast(STD_AST(std));
+            int new_std = STG_NEXT(astb.std);
+            if (new_std > MAXAST || astb.std.stg_base == NULL)
+              errfatal(7);
+            STD_AST(new_std) = new_ast;
+            A_STDP(new_ast, new_std);
+            STD_NEXT(prev_std) = new_std;
+            STD_PREV(new_std) = prev_std;
+            STD_NEXT(new_std) = next_std;
+            STD_PREV(next_std) = new_std;
+            prev_std = new_std;
+          }
+        }
+
         for (int std = STD_NEXT(do_std); std; std = STD_NEXT(std)) {
           STD_AST(std) = ast_rewrite(STD_AST(std));
         }

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.

6 participants