Merge elementwise implementations by niboshi · Pull Request #2920 · cupy/cupy

niboshi · 2020-01-07T18:54:20Z

Elementwise version of #2732.
Merges the logic of ElementwiseKernel and ufunc.

niboshi · 2020-01-09T06:45:28Z

@emcastillo
I think refactoring on elementwise-kernels is finished. May I hear a brief comment on the last commit (b746a0e)?
Some code is still duplicated (e.g. _preprocess_args) because it's also used from reduction.pyx.
If basic idea is OK, I'll try to update reduction.pyx too.
Reconsideration may happen at that moment.

emcastillo · 2020-01-09T06:47:03Z

Let me take a look

emcastillo · 2020-01-09T06:56:00Z

At first glance, the PR changes look fine and consistent.
But one of the things I am worried about is that with ufunc and elementwise being merged, it may complicate adding support for generalized functions since these can do reductions as well.

In numpy ufuncs are a special case of the generalized ufunc. (described by just the signature).
So I think that having a separated interface for the ufunc and argument checkings, signatures etc decoupled from the Elementwise kernel may keep things simpler. However, my knowledge of all these internals is limited and I am not sure of to what extent they can be a problem. @toslunar @asi1024 any comments on this?.

niboshi · 2020-01-09T07:07:22Z

@emcastillo
I agree that elementwise kernels and reduction kernels can be unified.
However, as it's difficult to do that all at once, you can consider this PR as a first step toward that.
(currently I don't have a concrete idea how they all will be unified though.)

So I think that having a separated interface for the ufunc and argument checkings, signatures etc decoupled from the Elementwise kernel may keep things simpler.

I don't understand this point.
My hope is all the kernel implementations (ufunc, ElementwiseKernel, _SimpleReductionKernel, ReductionKernel) should be able to be generalized to some extent.
Could you elaborate why you think ufunc and ElementwiseKernel should be separate?

emcastillo · 2020-01-09T07:08:37Z

If reduction and elementwise are going to be unified then there is no problem at all 😄
My fear was that gufunc combines both elementwise and reduction and if these two are not unified then lots of code would need to be duplicated.

emcastillo · 2020-01-09T09:54:24Z

This is a long one, so I just want to be sure that I am getting everything right.
What you have done here is:

Rewrite the kernel argument managing logic and abstract it into several classes instead of raw tuples and module level functions.
Create a base _AbstractElementwiseKernel that essentially groups all the ufunc related _kernel module functions.

I think it might be better to split this in two PRs, one with just _ArgKind, _ArgInfo and _TypeMap and another one with the rest of changes. But if you prefer to keep them as one PR I won't be against it.

niboshi · 2020-01-09T09:59:47Z

@emcastillo
Thank you for a comment.
I think your suggestion is very right. I'll separate 1. as another PR while this PR is kept (I mean this PR will depend on it).

emcastillo · 2020-01-09T10:04:09Z

Thanks!

niboshi · 2020-01-09T10:11:20Z

Btw, _Args class will be classified in 2., as it's abstraction between ufunc and ElementwiseKernel.

asi1024 · 2020-01-15T05:41:44Z

@niboshi Could you resolve conflicts?

niboshi · 2020-01-15T05:44:29Z

@asi1024 I'm planning to rebase after #2918.

pfn-ci-bot · 2020-03-25T09:04:35Z

Successfully created a job for commit 17bda99:

Dashboard for commit 17bda99

chainer-ci · 2020-03-25T10:41:12Z

Jenkins CI test (for commit 17bda99, target branch master) failed with status FAILURE.

asi1024 · 2020-03-25T11:41:26Z

Jenkins, test this please.

pfn-ci-bot · 2020-03-25T11:41:30Z

Successfully created a job for commit 7a7e582:

Dashboard for commit 7a7e582

chainer-ci · 2020-03-25T12:22:59Z

Jenkins CI test (for commit 7a7e582, target branch master) failed with status FAILURE.

asi1024 · 2020-03-25T18:09:08Z

Jenkins, test this please.

pfn-ci-bot · 2020-03-25T18:09:13Z

Successfully created a job for commit 43720e1:

Dashboard for commit 43720e1

leofang · 2020-03-25T18:15:43Z

cupy/cuda/function.pyx

+            # scalar
+            arg = ScalarArg(obj)
+            return arg
+        if isinstance(obj, TextureObject):


Thanks, @asi1024! Could you also include MemoryPointer in this category? I have an ongoing PR that would benefit having this. (I could also add this separately later.)

Suggested change

if isinstance(obj, TextureObject):

if isinstance(obj, (TextureObject, MemoryPointer)):

(need to import MemoryPointer first).

I'll fix it after your PR is merged. If this PR is merged before yours, please fix this line in your PR.

OK no worries 👍 I am still preparing it...

Thanks for your notification!

chainer-ci · 2020-03-25T18:48:12Z

Jenkins CI test (for commit 43720e1, target branch master) succeeded!

emcastillo · 2020-03-27T04:45:02Z

Benchmarked ufuncs,

Consistent increase in CPU time from 10us to 20us
This PR

time_sqrt    :    CPU:   20.238 us   +/- 0.263 (min:   19.956 / max:   20.900) us     GPU:   24.029 us   +/- 0.731 (min:   23.264 / max:   25.504) us

Master

time_sqrt   :    CPU:   11.371 us   +/- 0.247 (min:   11.143 / max:   11.853) us     GPU:   14.662 us   +/- 1.109 (min:   13.984 / max:   17.888) us

This time diff holds for all ufuncs, elementwise kernels, reduction and raw kernels.

emcastillo

LGTM

After discussing offline, we agreed to merge this PR now, as it is a blocker of other PRs.
We will investigate the reasons behind the performance degradation and address them after merging it.

This reverts commit ba7caa4, reversing changes made to e3ad9d4.

kmaehashi · 2020-04-23T06:43:42Z

Reverted in #3296 for performance issue.

niboshi force-pushed the refactor-elementwise branch 2 times, most recently from 768ab6a to 5ce38ca Compare January 7, 2020 19:52

niboshi assigned emcastillo Jan 8, 2020

niboshi force-pushed the refactor-elementwise branch 10 times, most recently from bd51dfe to b746a0e Compare January 9, 2020 06:28

asi1024 self-assigned this Jan 9, 2020

niboshi mentioned this pull request Jan 9, 2020

Enhance type and argument manipulation in elementwise and reduction kernels #2940

Merged

asi1024 added cat:code-fix Code refactoring that do not change behavior st:awaiting-author Awaiting response from author labels Jan 15, 2020

asi1024 added this to the v8.0.0b1 milestone Jan 28, 2020

niboshi force-pushed the refactor-elementwise branch from b746a0e to 3e0f181 Compare January 30, 2020 14:31

Introduced PointerArg for cuda.texture.TextureObject

7a7e582

Fix not to use _from_numpy_scalar(...) from outside

43720e1

leofang reviewed Mar 25, 2020

View reviewed changes

asi1024 changed the title ~~[WIP] Merge elementwise implementations~~ Merge elementwise implementations Mar 25, 2020

emcastillo approved these changes Mar 27, 2020

View reviewed changes

emcastillo added st:test-and-merge (deprecated) Ready to merge after test pass. and removed st:test-and-merge (deprecated) Ready to merge after test pass. labels Mar 27, 2020

emcastillo merged commit ba7caa4 into cupy:master Mar 27, 2020

niboshi deleted the refactor-elementwise branch March 27, 2020 10:54

leofang mentioned this pull request Mar 31, 2020

Performance boost: CUB-backed _SimpleReductionKernel #3244

Merged

7 tasks

This was referenced Apr 2, 2020

Remove unnecessary Arg instantiation in cuda/function.pyx #3253

Merged

Reduction fails with CUDA_ERROR_LAUNCH_OUT_OF_RESOURCES in some inputs #3258

Closed

leofang mentioned this pull request Apr 3, 2020

Possible to get / set gridDim and blockDim for custom Elementwise kernel? #3254

Closed

This was referenced Apr 9, 2020

Reduce arg creation cost in Arg.from_obj #3270

Merged

cpu-time performance degradation in _kernel.pyx #3279

Closed

emcastillo pushed a commit to emcastillo/cupy that referenced this pull request Apr 21, 2020

Revert "Merge pull request cupy#2920 from niboshi/refactor-elementwise"

fc8fc0c

This reverts commit ba7caa4, reversing changes made to e3ad9d4.

kmaehashi modified the milestones: v8.0.0b2, Closed issues and PRs Apr 23, 2020

leofang mentioned this pull request Jan 12, 2026

Revisit the splitting of the ufunc & ElementwiseKernel machineries #9586

Open

seberg mentioned this pull request Feb 17, 2026

MAINT,ENH: Refactor kernel argument handling into KernelArguments class #9705

Open

	if isinstance(obj, TextureObject):
	if isinstance(obj, (TextureObject, MemoryPointer)):

Uh oh!

Conversation

niboshi commented Jan 7, 2020

Uh oh!

niboshi commented Jan 9, 2020

Uh oh!

emcastillo commented Jan 9, 2020

Uh oh!

emcastillo commented Jan 9, 2020

Uh oh!

niboshi commented Jan 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

emcastillo commented Jan 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

emcastillo commented Jan 9, 2020

Uh oh!

niboshi commented Jan 9, 2020

Uh oh!

emcastillo commented Jan 9, 2020

Uh oh!

niboshi commented Jan 9, 2020

Uh oh!

asi1024 commented Jan 15, 2020

Uh oh!

niboshi commented Jan 15, 2020

Uh oh!

pfn-ci-bot commented Mar 25, 2020

Uh oh!

chainer-ci commented Mar 25, 2020

Uh oh!

asi1024 commented Mar 25, 2020

Uh oh!

pfn-ci-bot commented Mar 25, 2020

Uh oh!

chainer-ci commented Mar 25, 2020

Uh oh!

asi1024 commented Mar 25, 2020

Uh oh!

pfn-ci-bot commented Mar 25, 2020

Uh oh!

leofang Mar 25, 2020

Choose a reason for hiding this comment

Uh oh!

asi1024 Mar 25, 2020

Choose a reason for hiding this comment

Uh oh!

leofang Mar 25, 2020

Choose a reason for hiding this comment

Uh oh!

asi1024 Mar 25, 2020

Choose a reason for hiding this comment

Uh oh!

chainer-ci commented Mar 25, 2020

Uh oh!

emcastillo commented Mar 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

emcastillo left a comment

Choose a reason for hiding this comment

Uh oh!

kmaehashi commented Apr 23, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

niboshi commented Jan 9, 2020 •

edited

Loading

emcastillo commented Jan 9, 2020 •

edited

Loading

emcastillo commented Mar 27, 2020 •

edited

Loading