Assertion error when using "eval" & new UIUC speedup in TwoPunctures

Comments (18)

1. 

   Bernard Kelly reporter

   • removed comment

   More comments I forgot to include:

   • This was with -O3, as I said. If I use -O0 instead, the problem disappears.

   • My code was entirely the ET_2013_05 release, except the TwoPunctures thorn.

   • 2013-08-27T15:50:16+00:00
2. 

   anonymous

   • removed comment

   Suggested patch attached, relative to trunk, rev. 134. This involves a new d3tensor_SIMPLE and free_d3tensor_SIMPLE.

   This is a fairly minimal change. Really, the whole of TP_utilities should be overhauled, IMO.

   • 2013-09-04T19:00:57+00:00
3. 

   Erik Schnetter

   • removed comment

   The TwoPunctures thorn uses utility routines from the Numerical Recipes. The Fortran version of the NR uses standard Fortran, but the C version uses Fortran in C disguise -- a questionable choice. To be able to have array indices start with 1, all memory allocation functions can subtract an offset from the pointer, so that array[1] points to the first allocated element. (This is actually illegal in C, but probably works on all relevant systems.)

   Instead of introducing new *_SIMPLE functions, I would rather correct the existing function. Alternatively, the existing function could be removed, and be replaced with the new function.

   It seems the problem is that retval[nrl][0] is never set. The "apply all offsets" part is probably missing a loop.

   • 2013-09-04T22:37:54+00:00
4. 

   Roland Haas

   • removed comment

   Sorry for the tardy reply, the bug report was indeed very concise and clear.

   I agree that the memory management in TP_utilities is unnecessarily complex. It tries to be a drop in replacement for the (C-language) numerical recipes routines which allowed to have arrays to start with 1 instead of 0 (or any other offset for that matter). Not sure about the extra entry that they all allocate in each dimenension. It is possible that TP actually does us all elements from 0 to N inclusive (would need to check). Does anyone remember?

   The patch included however allocates the wrong type namely typeof(CCTK_REAL ) rather than typeof(CCTK_REAL) in line 48 of the patch. For typical machines this will go unnoticed since sizeof(CCTK_REAL)==sizof(double)==sizeof(void)==8.

   Otherwise it seems to be ok (baring issues that TP might use element "L" which I don't remember anymore).

   I wonder though if simply changing the original code as outlined in the description "the last factor should be (ndh-ndl+1), not (nrh-nrl+1)" would be sufficient? Mind you that I am the one responsible for the original buggy code should not have that much of an influence :-)

   • 2013-09-04T22:50:00+00:00
5. 

   Bernard Kelly reporter

   • removed comment

   Hi Roland. Apologies for the CCTK_REAL/CCTK_REAL* typeof; I'd corrected that myself locally, or so I thought.

   Anyway, I can confirm that fixing the one obvious logic bug in line 115 of the original code does not fix the actual issue; I'd tried that first in my own run. But it should be fixed, anyway, if the Numerical Recipes code is to persist.

   I suspect Erik's observation --- "It seems the problem is that retval[nrl][0] is never set." --- is the real problem in the original code. Nevertheless, I think the Numerical Recipes Fortran-masquerading-as-C code should be junked anyway. I was originally going to submit my simpler d3tensor as a replacement for what's there, but then thought that this would be more minimalist. That still leaves "dvector" etc ...

   • 2013-09-04T23:01:36+00:00
6. 

   Roland Haas

   • removed comment

   I unfortunately cannot reproduce the error. TP calls d3tensor with all lower bounds set to 0 so nrl=0 and retval[0][0] holds the return value from malloc.

   I made myself an extra paranoid version of d3tensor and ran a standalone copy (attached) through valgrind. Valgrind finds no errors. Can you perhaps run the standalone version on the cluster where the error occured, Bernard?

   I am also perfectly happy to replace d3tensor by d3tensor_SIMPLE by the way.

   • 2013-09-05T00:54:49+00:00
7. 

   Erik Schnetter

   • removed comment

   I attach a patch that shows how I think d3tensor could be corrected. The patch is untested -- it just shows where I think the logic errors are.

   • 2013-09-05T09:50:11+00:00
8. 

   Bernard Kelly reporter

   • removed comment

   Hi Erik. I just tried your patch from comment:7, but alas! --- same assertion error as before.

   Roland, I just tried your standalone test code from comment:6. It compiled & ran OK on the master node I compile on, though there were warnings. Here's my compilation line, which matches my standard CC config options for ET:

   icc -std=c99 -U__STRICT_ANSI__ -g -O3 -openmp -o d3tensor_TEST.exe d3tensor.c
   

   The warning messages are as follows (they're short enough not to require an attachment, I think), though they look pretty superficial:

   d3tensor.c(24): warning #181: argument is incompatible with corresponding format string conversion
       printf("d3tensor: %d %d %d\n",nrh,nch,ndh);
                                     ^
   
   d3tensor.c(24): warning #181: argument is incompatible with corresponding format string conversion
       printf("d3tensor: %d %d %d\n",nrh,nch,ndh);
                                         ^
   
   d3tensor.c(24): warning #181: argument is incompatible with corresponding format string conversion
       printf("d3tensor: %d %d %d\n",nrh,nch,ndh);
                                             ^
   
   d3tensor.c(27): warning #556: a value of type "double ***" cannot be assigned to an entity of type "void ***"
       a = retval;
         ^
   
   d3tensor.c(32): warning #556: a value of type "double **" cannot be assigned to an entity of type "void **"
       b = retval[0];
         ^
   
   d3tensor.c(81): warning #181: argument is incompatible with corresponding format string conversion
                  i, i/(nch-ncl+1), i%(nch-ncl+1));
                     ^
   
   d3tensor.c(81): warning #181: argument is incompatible with corresponding format string conversion
                  i, i/(nch-ncl+1), i%(nch-ncl+1));
   

   • 2013-09-16T14:15:19+00:00
9. 

   Roland Haas

   • removed comment

   Bernard: sorry I had not used the proper printf format specifiers to deal with the long int arguments. I have updloaded a corrected version that also accesses every single array element and still passes with valgrind on my workstation. The updated file is d3tensor.c.2 .

   • 2013-09-16T14:31:49+00:00
10. 

    Bernard Kelly reporter

    • removed comment

    Thanks, Roland. That removed the earlier warnings, but not the two from lines 27 & 32 [now lines 33 & 37] about the mismatch between double *** and void ***.

    But anyway, your standalone code compiles and runs with no sign of the assertion error I encounter when inside the actual TwoPunctures evaluation. I didn't actually mean for you to supply another test source file.

    • 2013-09-16T14:53:40+00:00
11. 

    Bernard Kelly reporter

    • removed comment

    Update. I think I've found the actual problem with the original code:

    Under -O3 optimisation (at least on this machine, with the Intel compiler), the pointer assignment loops weren't being completed before the assert statements were executed. I suspect this is because the assert statements don't look like they depend on the result of the assignment?

    I'm attaching a proposed patch to the repository TP_utilities.c that enforces the proper timing of the assert() checks by putting them in an IF statement that tests whether the loop iterator has actually reached the end of the loop. This may not be the best way of doing this, but it works for me, and still passes the test suites.

    • 2013-09-18T16:50:42+00:00
12. 

    Erik Schnetter

    • removed comment

    This is very strange. These additional if statements "protecting" the assert statements should not be necessary. This points to a severe problem in your compiler. Can you try with a different (newer) compiler, or with -O2 instead of -O3?

    • 2013-09-18T17:41:38+00:00
13. 

    Bernard Kelly reporter

    • removed comment

    Replying to [comment:12 eschnett]:

    This is very strange. These additional if statements "protecting" the assert statements should not be necessary. This points to a severe problem in your compiler. Can you try with a different (newer) compiler, or with -O2 instead of -O3?

    I've now tried it with -O2, and the problem doesn't appear to arise. I've reported it to the system admins to see what they say.

    • 2013-09-19T15:13:45+00:00
14. 

    Bernard Kelly reporter

    • removed comment

    So the admins' suggestion was to [drumroll] update the compiler version. The Intel compiler I was using --- v. 2013.1.117 (the latest available when I first hit this issue) --- had some problems with its -O3 "aggressive" optimisation. I went to the now-latest version installed --- v. 2013.5.192 --- and the problem disappeared for -O3.

    So I suppose this ticket should be closed. However, the logic bug in line 115 (noted in the original submission) should be fixed, short-term, and longer-term, I think this whole TP_utilities.c file should be replaced. Thanks for your help.

    • 2013-09-26T10:28:23+00:00
15. 

    Ian Hinder

    • removed comment

    I think we also had (or wanted to have) a mechanism for blacklisting certain compiler versions. It sounds like that version should be blacklisted, at least if TwoPuncture is present.

    • 2013-09-26T11:38:37+00:00
16. 

    Erik Schnetter

    • removed comment

    At the moment, an #ifdef in TwoPunctures would work fine. Later, we can move this to configure.in.

    • 2013-09-26T11:44:32+00:00
17. 

    Roland Haas

    • changed status to resolved
    • removed comment

    Applied as rev 135 of TwoPunctures.

    I've applied the patch to correct the amount of allocated memory "(nrh-nrl+1)" vs. "(ndh-ndl+1))". Bernard, would you mind letting me know what the output of icc --version is, please? The date in the first line is the compiler build date and it is accessible as the preprocessor varialbe __INTEL_COMPILER_BUILD_DATE . We would need this data for both the non-working version and the working one, to output a warning when the version is known to fail.

    The test might miss some if even version earlier than the one you tested fail and be too inclusive if the compiler bug goes away in a version earlier than 2013.5.192.

    I have opened a new ticket #1458 for this.

    • 2013-10-23T12:50:01+00:00
18. 

    Roland Haas

    • changed status to closed
    • edited description

    • 2019-02-21T19:53:12+00:00
19. Log in to comment