# Remove GPU warping, convolution, and support code

## Details

• Type: Story
• Status: Done
• Priority: Major
• Resolution: Done
• Fix Version/s: None
• Component/s:
• Labels:
• Templates:
• Story Points:
2
• Sprint:
DRP S17-1
• Team:
Data Release Production

## Description

Implement RFC-224.

## Activity

Hide
John Swinbank added a comment - - edited

Hi Perry,

First, most of what you've removed looks fine. Good job.

However, it seems like you have not only renamed convCpuGpuShared.cc, but also copied and pasted the only function it contains into BasicConvolve.cc. The new code in BasicConvolve.cc actually contains a note that it is the same as the code in BasicConvolve.cc: that is, it asserts that it is a copied & pasted version of itself! This clearly needs to be fixed.

I suggest that, since the "shared" in the old filenames refers to code being shared between CPU & GPU implementations, there's no longer any need to retain this convShared.cc file. Simply delete it, and the accompanying header, and migrate all the code into BasicConvolve.cc. Ensure you remove the weird comment about it being a copy & pasted version when you do so. You can put assertDimensionsOK in an anonymous namespace, since it's only referred to from within that file.

Other than that, I see quite a few remaining references to CUDA and/or GPUs in comments & setup and build scripts:

 [jds@magpie afw (tickets/DM-7669)]$grep -ri cuda * lib/SConscript:if env.ProductDir('cuda_toolkit') is not None: lib/SConscript: objs.append("#src/math/detail/cudaLanczos.os") src/math/detail/SConscript:if env.ProductDir('cuda_toolkit') is not None: src/math/detail/SConscript: envCudaCommon = env.Clone() src/math/detail/SConscript: envCudaCommon.Append(NVCCFLAGS = ' -gencode=arch=compute_13,code=\\"sm_13,compute_13\\" ') src/math/detail/SConscript: envCudaCommon.Append(NVCCFLAGS = ' -gencode=arch=compute_20,code=\\"sm_20,compute_20\\" ') src/math/detail/SConscript: envConv = envCudaCommon.Clone() src/math/detail/SConscript: envWarp = envCudaCommon.Clone() src/math/detail/SConscript: gpuObjConvolution = envWarp.SharedObject('#src/math/detail/cudaLanczos', ['#src/math/detail/cudaLanczos.cu']) ups/afw.cfg: "optional": ["cuda_toolkit"], ups/afw.table:setupOptional(cuda_toolkit)   [jds@magpie afw (tickets/DM-7669)]$ grep -ri GPU * | grep -v ipynb include/lsst/afw/math/detail/ConvolveShared.h: * @brief CPU and GPU convolution shared code include/lsst/afw/math/detail/PositionFunctor.h: * @brief GPU accelerared image warping lib/SConscript: objs.append("#src/math/detail/convGPU.os") src/math/detail/SConscript: gpuObjConvolution = envConv.SharedObject("#/src/math/detail/convGPU", ["#src/math/detail/convGPU.cu"]) src/math/detail/SConscript: gpuObjConvolution = envWarp.SharedObject('#src/math/detail/cudaLanczos', ['#src/math/detail/cudaLanczos.cu']) 

Please remove all of these before you merge.

Show
John Swinbank added a comment - - edited Hi Perry, A few comments: First, most of what you've removed looks fine. Good job. However, it seems like you have not only renamed convCpuGpuShared.cc , but also copied and pasted the only function it contains into BasicConvolve.cc . The new code in BasicConvolve.cc actually contains a note that it is the same as the code in BasicConvolve.cc : that is, it asserts that it is a copied & pasted version of itself! This clearly needs to be fixed. I suggest that, since the "shared" in the old filenames refers to code being shared between CPU & GPU implementations, there's no longer any need to retain this convShared.cc file. Simply delete it, and the accompanying header, and migrate all the code into BasicConvolve.cc . Ensure you remove the weird comment about it being a copy & pasted version when you do so. You can put assertDimensionsOK in an anonymous namespace, since it's only referred to from within that file. Other than that, I see quite a few remaining references to CUDA and/or GPUs in comments & setup and build scripts: [jds@magpie afw (tickets/DM-7669)]$grep -ri cuda * lib/SConscript:if env.ProductDir('cuda_toolkit') is not None: lib/SConscript: objs.append("#src/math/detail/cudaLanczos.os") src/math/detail/SConscript:if env.ProductDir('cuda_toolkit') is not None: src/math/detail/SConscript: envCudaCommon = env.Clone() src/math/detail/SConscript: envCudaCommon.Append(NVCCFLAGS = ' -gencode=arch=compute_13,code=\\"sm_13,compute_13\\" ') src/math/detail/SConscript: envCudaCommon.Append(NVCCFLAGS = ' -gencode=arch=compute_20,code=\\"sm_20,compute_20\\" ') src/math/detail/SConscript: envConv = envCudaCommon.Clone() src/math/detail/SConscript: envWarp = envCudaCommon.Clone() src/math/detail/SConscript: gpuObjConvolution = envWarp.SharedObject('#src/math/detail/cudaLanczos', ['#src/math/detail/cudaLanczos.cu']) ups/afw.cfg: "optional": ["cuda_toolkit"], ups/afw.table:setupOptional(cuda_toolkit) [jds@magpie afw (tickets/DM-7669)]$ grep -ri GPU * | grep -v ipynb include/lsst/afw/math/detail/ConvolveShared.h: * @brief CPU and GPU convolution shared code include/lsst/afw/math/detail/PositionFunctor.h: * @brief GPU accelerared image warping lib/SConscript: objs.append("#src/math/detail/convGPU.os") src/math/detail/SConscript: gpuObjConvolution = envConv.SharedObject("#/src/math/detail/convGPU", ["#src/math/detail/convGPU.cu"]) src/math/detail/SConscript: gpuObjConvolution = envWarp.SharedObject('#src/math/detail/cudaLanczos', ['#src/math/detail/cudaLanczos.cu']) Please remove all of these before you merge.
Hide
Perry Gee added a comment -

John Swinbank I didn't mean to checkin this change to BasicConvolve – I started to move the one routine there, then decided that I wouldn't move anything until I tried to get an opinion during review. I think that the detail directory is still oddly organized, with more modules than are really needed. However, I will make the change as suggested.

Show
Perry Gee added a comment - John Swinbank I didn't mean to checkin this change to BasicConvolve – I started to move the one routine there, then decided that I wouldn't move anything until I tried to get an opinion during review. I think that the detail directory is still oddly organized, with more modules than are really needed. However, I will make the change as suggested.
Hide
John Swinbank added a comment -

Hey Perry Gee: please remember to push your rebased ticket branch to github before you merge it with master. Otherwise, GitHub doesn't realise that it can close the PR.

Currently, your ticket branch seems to contain a bunch of weird obsolete commits, while the merge to master came from a user branch. That's unfortunate!

That minor complaint aside: thanks for getting this done!

Show
John Swinbank added a comment - Hey Perry Gee : please remember to push your rebased ticket branch to github before you merge it with master. Otherwise, GitHub doesn't realise that it can close the PR. Currently, your ticket branch seems to contain a bunch of weird obsolete commits, while the merge to master came from a user branch. That's unfortunate! That minor complaint aside: thanks for getting this done!
Hide
Perry Gee added a comment -

There are times when git does things I don't understand and can't fix. Right before I checked in, I did a pull, a git rebase master on the branch, and then a git merge --noff branch. I was left with a state where there were 19 commits on one and 20 on the other which differed. Since I could not figure out how to fix the problem, I started over on a different branch, did a diff of the final result, then merged the second branch to master.

Show
Perry Gee added a comment - There are times when git does things I don't understand and can't fix. Right before I checked in, I did a pull, a git rebase master on the branch, and then a git merge --noff branch. I was left with a state where there were 19 commits on one and 20 on the other which differed. Since I could not figure out how to fix the problem, I started over on a different branch, did a diff of the final result, then merged the second branch to master.
Hide
John Swinbank added a comment -

There are times when git does things I don't understand and can't fix.

Please don't hesitate to ask for help when this happens: I'd rather we spend a few extra minutes to get things right.

Show
John Swinbank added a comment - There are times when git does things I don't understand and can't fix. Please don't hesitate to ask for help when this happens: I'd rather we spend a few extra minutes to get things right.

## People

• Assignee:
Perry Gee
Reporter:
Jim Bosch
Reviewers:
John Swinbank
Watchers:
Colin Slater, Jim Bosch, John Swinbank, Perry Gee