fix the PORV grid property for clang with optimizations
for some reason, it looks like `std::find_if()` does not work on
ubuntu 16.10 with clang and enabled optimizations. since as far I can
see this 'if' is a minor performance improvement in non-time critical
code, let's just remove the condition.
What do you mean "doesn't work"? Does it not compile? Does it evaluate wrong?
it compiles, but I get NaNs in the values of the PORV grid property. this only happens on clang and only when optimizations are enabled. maybe clang has a problem with lambdas.
anyway, just removing the that "optimization" could even make the resulting code slightly faster, because if there are no nans in the array, the loop does the same thing as what the `find_if` has done before and the version after this patch only goes over the memory once...
I'm more worried about having to work through piles of compiler bugs than changing this, however, if there is a standards violation I'd like to know.
even if it was not required for clang, it would still be an improvement? (i.e., after this, we're jumping through less hoops than before!?)
Are you using `-ffast-math` for compilation?
> Are you using `-ffast-math` for compilation?
yes, but with an additional `-fno-finite-math-only`. I'm not sure if I tested without `-ffast-math`, but I'm sure that I downgraded the optimizations from `-Ofast -march=native` to `-O2` to no effect.
You are a brave man.
Well, that might be the cause for this, see the gcc manual page
When enabled, this option states that a range reduction step is not
needed when performing complex division. Also, there is no
checking whether the result of a complex multiplication or division
is "NaN + I*NaN", with an attempt to rescue the situation in that
case. The default is -fno-cx-limited-range, but is enabled by
This option controls the default setting of the ISO C99
"CX_LIMITED_RANGE" pragma. Nevertheless, the option applies to all
> Well, that might be the cause for this, see the gcc manual page
thanks, good to know. on my machine it currently works with GCC using the same flags as clang, but with your info considered, I suppose it could break after any GCC release.
Irrespective of whether this is strictly required, do you agree that the removal of the `find_if(); if()` statements result in (slightly) better code?
Not necessarily. With the `find_if` it still short-circuits the vector fetches somewhat more elaborate algorithm with a not-so-obvious fall-through for non-NaNs¹, and clarifies intent a bit (even though the algorithm itself is structured poorly, I might fix that some other time). Also, the right function to use would be `std::all_of`. By using `find_if` we can even speed it up a bit more, achieving single pass, by starting the counter at the first NaN.
¹ I chuckled typing that.
> Not necessarily. With the find_if it still short-circuits the vector fetches somewhat more elaborate algorithm with a not-so-obvious fall-through for non-NaNs¹,
did you check this or is this just speculation? (I.e., on one hand I'm willing to bet a beer on the fact that there is no noticeable performance difference for any real world deck, while on the other hand, the proposed version clearly is simpler and thus I'll claim that it is better from a maintainability POV.)
Hmmm - this should be green om travis now?
> Hmmm - this should be green om travis now?
right. the great shared_ptr purge probably came into the way (the error seems to be unrelated,though: it's in opm-upscaling, and about some *ConstPtr in a PVT object.) I've rebased this PR, maybe it helps...