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

Fparser app #366

Merged
merged 2 commits into from Oct 2, 2014
Merged

Fparser app #366

merged 2 commits into from Oct 2, 2014

Conversation

roystgnr
Copy link
Member

@roystgnr roystgnr commented Oct 1, 2014

This has been invaluable for boiling down problems with fparser strings.

I'm about to see if it's also useful for hunting down a bug (regression?) in fparser itself - try running "fparser_parse-dbg 'b := 1/abs(x); if(b<=1000,b,10)' 1"...

This is meant to be useful for debugging problems with fparser
strings, and right now it seems useful for debugging problems with
fparser...
@moosebuild
Copy link

Results of testing de5cbc2 using libmesh_PR_test recipe:

Passed on: linux-gnu

View the results here: https://www.moosebuild.com/view_job/6106

@roystgnr
Copy link
Member Author

roystgnr commented Oct 1, 2014

That fparser line works fine with a --disable-fparser-optimizer build.

@moosebuild
Copy link

Results of testing de5cbc2 using libmesh_PR_test_dbg recipe:

Passed on: linux-gnu

View the results here: https://www.moosebuild.com/view_job/6107

@jwpeterson
Copy link
Member

@dschwen

@dschwen
Copy link
Member

dschwen commented Oct 1, 2014

Huh?

@jwpeterson
Copy link
Member

Huh?

Would you care to comment on some of the fparser optimizer debugging methods you've developed over time?

@dschwen
Copy link
Member

dschwen commented Oct 1, 2014

I had to look at ParsedFunction again to see how much of the new FParserAD stuff it supports. It might be worthwhile to add a call to gradient() to that app and output that as well. And while you're at it add a range/step parameter. That way the app could be used for quick tabulation, or writing tests.

Maybe I should add an interface to ParsedFunction to perform the just in time compilation. That functionality is not exposed by ParsedFunction yet. It could be added to this app (optional) as well. That way the app could be used to populate the JIT cache on systems that do not provide compilers on compute nodes.

@roystgnr
Copy link
Member Author

roystgnr commented Oct 1, 2014

How hard would it be to add JIT support to ParsedFunction? I'm currently between a rock and a hard place; runs with --disable-fparser-optimizer are intolerably slow for our app, runs with the default configuration give grossly wrong results under some conditions.

@roystgnr
Copy link
Member Author

roystgnr commented Oct 1, 2014

This doesn't look like an easy-to-bisect regression, either - I get the same failure when backporting this PR onto f58d4a4...

@dschwen
Copy link
Member

dschwen commented Oct 1, 2014

Adding jit support would be as easy as adding a compile() member that invokes the jitcompile method on the contained fparser object. But I guess I don't understand what the regression is yet.

@roystgnr
Copy link
Member Author

roystgnr commented Oct 1, 2014

The command I listed above should obviously return 1, and if you make slight simplifications (get rid of the abs() call or the inversion) it does; instead it returns 10.

@dschwen
Copy link
Member

dschwen commented Oct 1, 2014

Ok, I realized that in the mean time and I'm building a little test program to look at the bytecode before and after optimization. I have a suspicion that it may be related to optimizer rules regarding 'guaranteed positivity' (which is set by the abs function). Let me check.

@dschwen
Copy link
Member

dschwen commented Oct 1, 2014

Nice.... this is what my thingie spits out:

Input Expression:
b := 1/abs(x); if(b<=1000,b,10)
Size of stack: 3
0000: push Var0     = [0]x
0001: abs           = [0]abs(x)
0002: inv           = [0]abs(x)^-1
0003: dup           = [1]abs(x)^-1
0004: push 1000     = [2]1000
0005: le            = [1]le(abs(x)^-1,1000)
0006: jz_abs 1,000d = (void)
0009: dup           = [1]abs(x)^-1
000a: jump 2,000e   = (void)
000d: push 10       = [1]10
000e: ----- (phi)   = [1]if(le(abs(x)^-1,1000),abs(x)^-1,10)

f(1) = 1

Optimized Input Expression:
b := 1/abs(x); if(b<=1000,b,10)
Size of stack: 1
0000: push 10       = [0]10

f(1) = 10

The FPOptimizer is waaay overeager here and optimizes the program down to a constant. I will investigate further.

@jwpeterson
Copy link
Member

On Wed, Oct 1, 2014 at 1:06 PM, Daniel Schwen notifications@github.com
wrote:

Nice.... tjis is what my thingie spits out:

Input Expression:
b := 1/abs(x); if(b<=1000,b,10)
Size of stack: 3
0000: push Var0 = [0]x
0001: abs = [0]abs(x)
0002: inv = [0]abs(x)^-1
0003: dup = [1]abs(x)^-1
0004: push 1000 = [2]1000
0005: le = [1]le(abs(x)^-1,1000)
0006: jz_abs 1,000d = (void)
0009: dup = [1]abs(x)^-1
000a: jump 2,000e = (void)
000d: push 10 = [1]10
000e: ----- (phi) = [1]if(le(abs(x)^-1,1000),abs(x)^-1,10)

f(1) = 1

Optimized Input Expression:
b := 1/abs(x); if(b<=1000,b,10)
Size of stack: 1
0000: push 10 = [0]10

f(1) = 10

The FPOptimizer is waaay overeager here and optimizes the program down to
a constant. I will investigate further.

Cool, this is really good software.

John

@dschwen
Copy link
Member

dschwen commented Oct 1, 2014

You can construct similar cases with the < operator: 1/abs(x)<1 for x==1 should evaluate to 1, but optimizes to 0. It is like the argument of the < and <= operators are ignored and assumed to be 0.

@jwpeterson
Copy link
Member

@roystgnr may also get a kick out of idaholab/moose#1923

@roystgnr
Copy link
Member Author

roystgnr commented Oct 1, 2014

No luck switching to JIT. Here's the code generated:

#define _USE_MATH_DEFINES
#include <cmath>
extern "C" double f_c3da290ffb3d00003aa5165348aa0c85a6852fc8(const double *params, const double *immed, const double eps) {
double r, s[2];
s[0] = params[0];
s[0] = std::abs(s[0]);
s[0] = 1.0/s[0];
s[1] = immed[0];
s[0] = s[0] < (s[1] - eps);
if (s[0] < 0.5) goto l12;
s[0] = params[0];
return s[0]; }
l12:
s[0] = immed[1];
return s[0]; }

Which obviously causes the compiler to scream and run away from the screwy braces.

@dschwen
Copy link
Member

dschwen commented Oct 1, 2014

Oh, yep. I see exactly where this bug is in the code. One sec...

@permcody
Copy link
Member

permcody commented Oct 1, 2014

In my experience "One sec" usually equates to one bug fixed, one new bug
introduced...

Hold onto your butts!

On Wed, Oct 1, 2014 at 2:50 PM, Daniel Schwen notifications@github.com
wrote:

Oh, yep. I see exactly where this bug is in the code. One sec...


Reply to this email directly or view it on GitHub
#366 (comment).

@jwpeterson
Copy link
Member

On Wed, Oct 1, 2014 at 3:10 PM, Cody Permann notifications@github.com
wrote:

In my experience "One sec" usually equates to one bug fixed, one new bug
introduced...

Hold onto your butts!

Heheh. It was a one character change, hopefully it doesn't break anything
else.

John

@dschwen
Copy link
Member

dschwen commented Oct 1, 2014

It was a pretty obvious mistake (that should have been spotted during code review, I mean.. come on guys! ;-) )

@jwpeterson
Copy link
Member

On Wed, Oct 1, 2014 at 3:13 PM, Daniel Schwen notifications@github.com
wrote:

It was a pretty obvious mistake (that should have been spotted during code
review, I mean.. come on guys! ;-) )

I blame poor test coverage.

John

@roystgnr
Copy link
Member Author

roystgnr commented Oct 1, 2014

We ought to use this app to make a big "fparser test script" eventually. First we'd want to expose decisions like whether to call Optimize() and/or JITCompile() at run-time rather than at configure-time (or as in my current JIT test builds, at "roy just changes parsed_function.h and sees if it works"-time).

@jwpeterson
Copy link
Member

Seems more like a CPPUNIT test to me.

@roystgnr
Copy link
Member Author

roystgnr commented Oct 1, 2014

@dschwen, another JIT bug with a 1 char fix: you've got atan2 calls getting converted to atan calls.

@dschwen
Copy link
Member

dschwen commented Oct 1, 2014

Thx, @roystgnr will fix that in a minute.

@dschwen
Copy link
Member

dschwen commented Oct 1, 2014

Coming back to the original issue. I think I'm zeroing in. The culprit is a faulty result boundary calculation. FParser seems to think that the minimum possible value for 1/abs(x) is inf :-/

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.

None yet

5 participants