Feature/simd for multi rhs

mentioned in merge request dune-common!16 (merged)

I'm hoping for comments here

Moving the general discussion to the issue where it belongs and not cluttering the merge request any longer.

The reference stuff was meant as a wish for improvement, independent of the other part of the message.

N4184 and N4185 were not in shape last year, I don't know whether this has changed.

I am opposed to solution that make it into Dune, that are used by very few people. The feature might be great, but we have a lot of code only these few people take care of. As a release manager it is hard to test all these balconies. And if bugs appear, only few people can or are willing to do so. And once some code becomes part of Dune, it is very difficult to get rid of it. If other developers are in favor of your VC bindings, I do not oppose your merge request. I like the idea of using SIMD for free. Let's see how others feel.

@gruenich I have the feeling you added the comment to the wrong merge-request?

I like the idea of using SIMD for free.

I'm not sure I understand what you imply here? Do you want to say the compiler does this for free, or does "for free" refer to the proposed feature?

Regarding "only few people". I think you miss a major fact. Many people use things like MS-FEM and all these users immediately profit from such a feature once the high-level parts, like fem, pdelab or dumux pick this up. Apart from this there are already two projects, one in my group and one in Marios group where we need this feature. There are many places in DUNE where we will need support for SIMD, but the changes will be far more invasive than in this simple case, thus I'm not suggesting to merge them soon.

Immediate questions popping up is:

Does it work in parallel? I guess it should as VC vector size seems static and the default MPITraits should work, right? Anyway in the long run we need to add a proper MPITrait for it.

In your experience are the number of iterations till convergence that similar for the different right hand sides that this pays off?

Do you have any performance numbers?

For direct methods this makes totally sense. As SuperLU supports multiple right hand sides it propbably should support it.

Concerning AMG I guess it would suffice to port the Transfer classes that prolongate and restrict. I can hardly imagine that there could be other places that matter.

BTW: The above discussed extension look like a nice low hanging GSoc/ESoc project. I am sure that there would be a few applications for it.

@markus.blatt I did tests with random RHS and it behaves as expected. As the convergence rate for the CG only depends on the condition number of the operator, it should (in theory) be independent from the RHS (as we do the orthogonalization separately for each RHS). This is also the observed behavior. Let it differ by one iteration in practical tests.

I didn't test in parallel. You are right the correct MPITraits are something we will need on the long run.

Rearding the AMG I agree. I don't expect many problems, but I didn't even try to run it yet.

Added 3 commits:

871edb71 - [test] changes in Vc force us (atm) to skip any mixed-precision tests
8092a52c - [amg] allow to explicitly disable the coarse direct solver
25ccf8d5 - [test] multirhs also works for amg

Nice update, when disabling the coarse direct solve, the test immediately works to the AMG.

Can we decide whether we want (to keep) Vc in common/istl? I still think the name is utter nonsense. Ever tried googling Vc? CMake is right, venture capital cannot be found on my hard-drive.

Can we set up a vote or similar? Time is pressing as people are already fixing the accidentally merged branch.

If this was merged by mistake and without consensus, and if it already breaks things, then let's get it out again. Why can't we simply git revert the offending patches? After a proper discussion we can still decide to take it back in again.

mentioned in issue dune-common#26 (closed)

@gruenich

CMake is right, venture capital cannot be found on my hard-drive.

how about beeing a bit more explicit?

I don't care about the name, but I do care about the functionality. I definitely want that feature in. We can discuss about design decision and so on, but up to now you never brought up a reason with regards to content, I think we should rather fix arising issues instead of creating an utter mess by reverting back and forth.

I am against adding third party libraries virtually nobody is using to the core modules. It becomes a personal feature only few people care and maintain. Unless several people want (to use) this feature, I prefer to put in a dune module of its own.

Features introduced, used and maintained by too few people were/are IMHO: psurface, amiramesh, grape, netbib (some function spaces stuff formerly used in geometry and localfunctions). Some were removed, some moved to dedicated dune modules, some bitrot.

As a release manager it is difficult to get these dependencies. I never got netbib running, Grape and Amiramesh are a major hassle.

Overall I don't see that people are overly enthusiastic about Vc. Markus showed some interest, Jö seems to us it. Seems a perfect fit for an additional dune module, doesn't it?

You are repeating yourself and still don't contribute anything new to the discussion. You are currently stating that nobody would use this feature, which is totally ungrounded, while there are imho sufficient options to use it and there are already different potential users. It can also not be implemented externally as it requires some minor, but explicit changes in istl to actually use it and I hope you are not actually suggesting to fork major parts of istl. Also you are the only one arguing against it.

And you are the only one arguing in favor of it.

I don't have any more arguments. I am concerned about the lack of interst by other developers. If several developers state that they are ok with the merge and are interested in this feature, I shut up.

I intend to use for grid patches. The problem for me is that I need at least some support in the core modules -- specifically dune-geometry.

I need a version of MultilinearGeometry where I can use SimdArray<double, vector_lanes> as the ctype. MultilinearGeometry uses a class MatrixHelper, which includes code like

assert( xDiag > FieldType( 0 ) );

Here, xDiag and FieldType would be SimdArrays, so the result of the comparison is not a simple bool, but a vector of bools. And we want the assert() to trigger if any of the bools in the vector is false. The problem here is that there is no canonical way to interpret this vector of bools as a single bool. In this case you want to use && when accumulating the vector entries, but there are other cases where || is appropriate. There are even cases where you now need to enter both branches of an if statement if the condition vector contains both true and false values, and you need to mask the operations within each branch. There are ways to do that, but still, hopefully those cases will not occur too often.

To express that correctly the above code needs to be modified:

assert( all_true( xDiag > FieldType( 0 ) ) );

This uses the syntax that @christi introduced in dune-common!16 (merged).

This illustrates why I cannot do this purely as an external library. For the MatrixHelper in dune-geometry to be able to use all_true() there must be at least all_true(bool) available in dune-geometry or dune-common, so the matrix helper works with the fundamental types. If not, I can of course still implement my own version of the MatrixHelper in my own module and instantiate the MultiLinearGeometry with that, but that would essentially be a copy of the one from dune-geometry with all_true() added or the assert() removed. And since the MatrixHelper won't be the only place where modifications are needed, over time I'd end up copying large parts of the core modules into my own module.

Now, Vc is not the only vectorization library out there. So it is not totally absurd to keep things like all_true(bool) in dune-common, but to put all_true(Vc::MaskArray) (if that was the correct name) into some bindings modules (e.g. dune-vc). That way support for e.g. vectorclass could go into another bindings module. But we still need the basic operations for the fundamental types in dune-common, even if they do essentially nothing, because those are the hooks for the bindings modules.

The reason why I'm not 100% certain about this is that I do know Vc, but I don't really know any of the other vectorization libraries. They will all follow similar ideas, but I am not sure whether it is possible to design hooks that will suit them all. In particular when masked operations are needed.

Having a closer look, trying to figure out which parts of dune-common!16 (merged) could go into a bindings module and which ones would need to stay in dune-common:

dune-common: conditional.hh, rangeutilities.hh, the typetraits.hh snippet
Vc bindings module: AddVcFlags.cmake, the actual find_package(Vc) call, the config.h.cmake snippet, simd.hh

First something on the fundamental discussion: I think it is reasonable to have "experimental features" in the core even if at first they will only be used by a few people. They should just be set up in such a way that potentially a large group of people could use them, i.e., they shouldn't be special features that then later on are only usably through dune-fem, pdelab or some other single module - or at least in that case this needs to be discussed separately. The release manager's job should not be made more difficult by this, i.e., they are experimental and consequently will not always work. It's not only release management but simple changes to the core that will now and again break these experimental features (just adding another assert like the one in the MatrixHelper would do that and developers can't be expected to test features like this when adding such an assert). That must be the understood risk of people using them.

This request started off with a use case in dune-istl and the changed could then perhaps be made there; but now it seems to have broadened out to "grid patches". What is the use case here?

Regarding the MR for dune-istl (and potential MR for other core modules): this is tricky. It is kind of important to test istl with a vectorization library if istl supports such a thing, to make sure you did not write all_true() when you meant to write any_true(). This is something that tests that only use the fundamental types cannot detect. It is probably not necessary to test this all the time, but it would still mean that istl must suggest either some vectorization module directly or the corresponding bindings module. Alternatively, we could make a module dune-istl-vc-tests or similiar, but the path leads to an epic proliferation of modules.

A third alternative would be some kind of dummy vectorization library, that offers the same interface but does not use vectorization primitives, just plain for-loops, and which is included in dune-common. This should be sufficient for testing.

Feature/simd for multi rhs

Merge request reports

Activity