devitocodes / devito

DSL and compiler framework for automated finite-differences and stencil computation
http://www.devitoproject.org
MIT License
562 stars 228 forks source link

DSE behavior #206

Closed mloubout closed 7 years ago

mloubout commented 7 years ago

This issue is about a few problematic currently encountered with dse. 1- 2D TTI segfault in Legacy mode with dse=advanced 2- 2D TTI runs with no Legacy and dse=advanced but produces wrong results and has warnings at compilation

/var/folders/z0/tqrtpj3j5lz5vmpm09lx4mbw0000gp/T/devito-502/b8303fc358a68d6a5ac6966a507c6548bcc76c88.c:82:20: warning: unused variable 'src_coords' [-Wunused-variable]
   float (*restrict src_coords)[d_size] __attribute__((aligned(64))) = (float (*)[d_size]) src_coords_vec;
                    ^
/var/folders/z0/tqrtpj3j5lz5vmpm09lx4mbw0000gp/T/devito-502/b8303fc358a68d6a5ac6966a507c6548bcc76c88.c:81:20: warning: unused variable 'src' [-Wunused-variable]
   float (*restrict src)[1] __attribute__((aligned(64))) = (float (*)[1]) src_vec;
                    ^
/var/folders/z0/tqrtpj3j5lz5vmpm09lx4mbw0000gp/T/devito-502/b8303fc358a68d6a5ac6966a507c6548bcc76c88.c:78:20: warning: unused variable 'm' [-Wunused-variable]
   float (*restrict m)[y_size] __attribute__((aligned(64))) = (float (*)[y_size]) m_vec;
                    ^
/var/folders/z0/tqrtpj3j5lz5vmpm09lx4mbw0000gp/T/devito-502/b8303fc358a68d6a5ac6966a507c6548bcc76c88.c:77:20: warning: unused variable 'epsilon' [-Wunused-variable]
   float (*restrict epsilon)[y_size] __attribute__((aligned(64))) = (float (*)[y_size]) epsilon_vec;
                    ^
/var/folders/z0/tqrtpj3j5lz5vmpm09lx4mbw0000gp/T/devito-502/b8303fc358a68d6a5ac6966a507c6548bcc76c88.c:76:20: warning: unused variable 'delta' [-Wunused-variable]
   float (*restrict delta)[y_size] __attribute__((aligned(64))) = (float (*)[y_size]) delta_vec;
                    ^
/var/folders/z0/tqrtpj3j5lz5vmpm09lx4mbw0000gp/T/devito-502/b8303fc358a68d6a5ac6966a507c6548bcc76c88.c:75:20: warning: unused variable 'damp' [-Wunused-variable]
   float (*restrict damp)[y_size] __attribute__((aligned(64))) = (float (*)[y_size]) damp_vec;
FabioLuporini commented 7 years ago

The warning are an artefact of the current code generation system. They will disappear in a future release. We could temporarily add this option -Wunused-variable to the compilation line.

Could you tell me a bit more about these wrong results.

First of all, I need instructions on how to reproduce this (branch, command line, etc.) What is the result that you expect, and what instead are you getting? Maybe tell me just the value of a norm ? I get that with dse = basic this works OK ?

mloubout commented 7 years ago

First of all, I need instructions on how to reproduce this (branch, command line, etc.)

this is the plots I sent you in slack, mainly, run TTI with and without dse and you'll see the result without is "fine" while the result with is "noisy"

FabioLuporini commented 7 years ago

since you're not using the master branch, I need:

And I don't know how to generate the plots.

In these cases, you should write a minimal failing example and output a norm (for example) so that the only thing I have to look at is a number.

Does the problem disappear with dse=basic? Does it disappear with dse=noop ?

mloubout commented 7 years ago

This is master branch.

Does the problem disappear with dse=basic? Does it disappear with dse=noop ?

only tried advanced and None

FabioLuporini commented 7 years ago

OK, so, if you now merge devito3-final, the problem should just disappear in non-legacy mode. The segfault in legacy more seems to be still there, but I don't think investing time in fixing it is a good idea.

I rather encourage to progressively move everything to the "new" Devito3 API, and report any other bugs in case you found more. The test_Marmousi that you provided offline was excellent.

Please confirm that this is now fixed in non-legacy mode

mloubout commented 7 years ago

Once again, everything we run/will run at UBC is gonna stay with Legacy untill we have adjoints and gradients and making changes that forces us to work with 2-3 month old version of devito is gonna be dredfull to update once devito 3.0 is in. SO yes I still need legacy to run properly.

FabioLuporini commented 7 years ago

So just switch to dse='basic' or even dse=None for now. As you're just developing, you don't need performance ATM. I think it's safe to say that this issue has lower priority; the bug is probably caused by some stupid interaction with the old propagator -- perhaps I'll check this next week. Thoughts, @mlange05 @navjotk ?