search

lattice / quda

QUDA is a library for performing calculations in lattice QCD on GPUs.
https://lattice.github.io/quda
Other
279 stars 94 forks source link

Solvers should use ColorSpinorField instead of cudaColorSpinorField #472

Open mathiaswagner opened 8 years ago

mathiaswagner commented 8 years ago

Files in which grep found cudaColorSpinorField and hence might require closer investigation and updating to ColorSpinorField:

maddyscientist commented 8 years ago

No, they should all use ColorSpinorField. Just a relic of history. I've only switched the ones over that were needed for multigrid, and haven't gotten around to the rest (yet).

mathiaswagner commented 8 years ago

Ok. I can take care of (at least some of ) them. I'll add a list of solvers that require updates later.

mathiaswagner commented 8 years ago

I created hotfix/cleanup_ColorSpinorField for that so feel free to add to it. @alexstrel: It's probably best if you check what is need for the deflated solvers.

AlexVaq commented 8 years ago

I'm doing the covariant derivative and some minor routines I added. Since there is no CPU implementation of any of these, I'll just exit if the spinor field is not in the GPU.

I have a nother problem updating the ghost exchange. Previously we agreed I would use ColorSpinorField::exchangeGhost() instead of my custom implementation, but I didn't find this function. I found cudaColorSpinorField::exchangeGhost(), though, but I can't use that for spin 4 unprojected spinors.

Is there any way to do this, or should I stay with the custom routine? It would be nice to add everything to the cleanup.

On Wed, May 25, 2016 at 6:33 AM, Mathias Wagner notifications@github.com wrote:

Files in which grep found cudaColorSpinorField and hence might require closer investigation and updating to ColorSpinorField:

  • lib/inv_cg3_quda.cpp who - comment
  • lib/inv_cg_quda.cpp who - comment
  • lib/inv_eigcg_quda.cpp who - comment
  • lib/inv_gcr_quda.cpp who - comment
  • lib/inv_gmresdr_quda.cpp who - comment
  • lib/inv_mpbicgstab_quda.cpp who - comment
  • lib/inv_mpcg_quda.cpp who - comment
  • lib/inv_multi_cg_quda.cpp who - comment
  • lib/inv_pcg_quda.cpp who - comment
  • lib/inv_sbicgstab_quda.cpp who - comment
  • lib/inv_sd_quda.cpp who - comment
  • lib/inv_xsd_quda.cpp who - comment

I created hotfix/cleanup_ColorSpinorField for that so feel free to add to it. @alexstrel https://github.com/alexstrel: It's probably best if you check what is need for the deflated solvers.

— You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub https://github.com/lattice/quda/issues/472#issuecomment-221470751

maddyscientist commented 8 years ago

@alexstrel From color_spinor_field.h:

    /**
       This is a unified ghost exchange function for doing a complete
       halo exchange regardless of the type of field.  All dimensions 
       are exchanged and no spin projection is done in the case of 
       Wilson fermions.
    */
    virtual void exchangeGhost(QudaParity parity, int dagger) const = 0;

This is implemented for both cudaColorSpinorField and cpuColorSpinorField, and will work on unprojected fields.

AlexVaq commented 8 years ago

Mmm, ok. I suppose I was mixing the optimized routines for Wilson-type dslash with this generic one. I'll give it a try and update the covDev.

alexstrel commented 8 years ago

Ok, will check the deflated stuff.

AlexVaq commented 8 years ago

I'm trying to clean-up the covariant derivative. So far introducing ColorSpinorField instead of cudaColorSpinorField has been trivial, but when it comes to the ghosts I'm finding some trouble.

Following @maddyscientist recommendation, I use "in->exchangeGhost(parity, dagger);" to exchange ghosts before calling the covariant derivative, and as far as I understand, I should use the ghost texture GHOSTSPINORTEX and READ_SPINOR in the kernels, but this approach is failing (I get "illegal memory access"). I noticed that cudaColorSpinorField::exchangeGhost(parity,dagger) creates a ghost_fixme array, but I believe this is never binded to texture.

Is there a way to fix this, or it's just that the implementation is incomplete, and I should implement myself the binding of the ghost texture? It looks to me that this ghost_fixme was designed for multigrid and cpu vectors, and trivially extended to cuda spinors without tackling the texture issues, but I might be wrong.

maddyscientist commented 8 years ago

Ah your assessment is completely correct. Looks like I need to merge to the two ghost arrays before you use this to have both generic exchange and textures. I'll plan on doing this next week if I can, it was on the to do list anyway since this is needed to support p2p for multigrid operators. Sorry about this.

AlexVaq commented 8 years ago

No problem, then I might very well implement ColorSpinorFields everywhere without touching the ghost exchange implementation, and after you fix this I’ll clean-up the covariant derivative.

Thanks for reporting. I think you saved me a lot of time.

AlexVaq commented 8 years ago

I see that not all solvers are listed in @mathiaswagner list. Is there any reason for that? Also, why is inv_cg3_quda.cpp in the list (or why is that file in the library)? It looks pretty useless at this stage.

mathiaswagner commented 8 years ago

inv_cg3_quda.cpp is indeed outdated. I think Justin put that in as a simple reference for developing the s-step solver. My list was based on grepping for cudaColorSpinorField and files that don't show up in my list should be using 'ColorSpinorField' already. But I might have missed a file. Please feel free to add to the list.

AlexVaq commented 8 years ago

I marked the GCR as done, because I think @maddyscientist updated it (and my inspection of the file tells me so). Can you confirm, @maddyscientist ?

Also, I'm removing inv_cg3_quda.cpp from the list. Maybe we should remove it from the library as well.