zynaddsubfx / zyn-fusion-build

Build Scripts For Zyn-Fusion
Do What The F*ck You Want To Public License
122 stars 39 forks source link

GCC optimization bug(?) for "file_list_dirs" port #65

Closed ghost closed 3 years ago

ghost commented 3 years ago

In commit https://github.com/zynaddsubfx/zynaddsubfx/commit/22a5f8da76c16c47c3814eedaec8f9fe0ff4248b there's a fix for gcc 10.1.0. I'm using gcc 10.2.0 in msys and I think I'm hitting the same bug. This compiler inlined the function gcc_10_1_0_is_dumb and used xmm registers. If I disable the optimization using pragmas the problem went away:

#pragma GCC push_options
#pragma GCC optimize("O0")
void
gcc_10_1_0_is_dumb(const std::vector<std::string> &files,
        const int N,
        char *types,
        rtosc_arg_t *args)
{

        types[N] = 0;
        for(int i=0; i<N; ++i) {
            args[i].s = files[i].c_str();
            //printf("gcc dumb %d: value %p\n", i, args[i].s);
            types[i]  = 's';
        }
}
#pragma GCC pop_options

The pragmas should probably be surrounded by an ifdef to check if it's gcc. Maybe even check for a specific version?

fundamental commented 3 years ago

Given that essentially no-one compiles zyn with anything other than modernish gcc/clang (i.e. not msvc), and this bug only occurs in very recent gcc versions only when the function is inlined, perhaps just adding a no-inline declaration on the function should save things?

If I had the energy to do so I'd report another compiler bug, but reducing "moving pointers from a class method into an array of unions which is only incorrectly optimized in a lambda callback within an initializer list" to a minimal test case, somehow isn't high on my list of fun tasks :p

I think __attribute__ ((noinline)) is the current way of specifying that a function should never be inlined and applies to clang/gcc, though that's based off my C++11 style approaches. I'm not sure if clang/gcc figured out something better in the C++14/C++17 era.

ghost commented 3 years ago

Using __attribute ((noinline)) also works when I compile it.

I know I'm being nitpicky and annoying by saying this, but I prefer the explicit denial of optimizations by using pragmas. I think you made it a separate function so gcc wouldn't use an optimization which produces faulty code? Whether the code is inlined or not isn't the issue, the faulty optimization is the issue.

With some bad luck, the gcc team does some nifty flow optimizations and that function gets hit with the same optimization bug although it isn't inlined. Using the pragma explicitly tells the compiler "No! Do not optimize that function because you'll probably screw it up"

Anyways, both options (pragma & noinline) both produce a working executable. So either is fine by me. I only wanted to present a case for using the pragmas.

reducing "moving pointers from a class method into an array of unions which is only incorrectly optimized in a lambda callback within an initializer list" to a minimal test case, somehow isn't high on my list of fun tasks :p

Heh... This could explain why nobody comes to my game nights anymore :)

fundamental commented 3 years ago

I think you made it a separate function so gcc wouldn't use an optimization which produces faulty code?

100% correct, the buggy optimization only occurred when it was inline within the callback.

the gcc team does some nifty flow optimizations and that function gets hit with the same optimization bug although it isn't inlined.

Well, that would sure save me some time in presenting a minimal example for the gcc devs :p (I'm 99.5% sure there's no undefined/implementation defined/ill-defined/etc behavior in the block)

I only wanted to present a case for using the pragmas.

Sure thing, my preference was pretty weak, so I'll let your opinion override mine. As long as the right #ifdef THIS_IS_GCC is added around the pragma that should be fine. Feel free to add a PR with that and I'll merge it fairly fast, otherwise I'll add in the code myself when I circle back to this issue. (goodness, zyn related email can really build up when you're not paying close attention on a mostly daily basis)

Heh... This could explain why nobody comes to my game nights anymore :)

Don't get me wrong. I'll totally sign up for Bugzilla The Board Game (TM) as soon as this country figures out how to handle a pandemic. Most of my cardboard is just gathering dust now... (and Castles of Burgundy is still in its shrink wrap :-/)