`addmm`: port to structured kernel #57417

walterddr · 2021-05-01T14:24:57Z

Port addmm to structure kernel

Follow ups

migrate mm and addbmm to structure
move TORCH_CHECKS currently in addmm_cpu_impl_ and addmm_out_cuda_impl to meta

facebook-github-bot · 2021-05-01T14:25:04Z

💊 CI failures summary and remediations

As of commit 335025a (more details on the Dr. CI page):

💚 💚 Looks good so far! There are no failures yet. 💚 💚

This comment was automatically generated by Dr. CI (expand for details).

Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

codecov · 2021-05-06T04:31:26Z

Codecov Report

Merging #57417 (335025a) into master (c911c30) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master   #57417   +/-   ##
=======================================
  Coverage   76.84%   76.84%           
=======================================
  Files        1986     1986           
  Lines      197902   197902           
=======================================
+ Hits       152079   152081    +2     
+ Misses      45823    45821    -2

ezyang · 2021-05-07T03:28:10Z

aten/src/ATen/NamedTensorUtils.cpp

This is a little bit of a pessimization but not by much since the internals of this functional already always allocated a vector.

seems already discussed in: https://github.com/pytorch/pytorch/pull/55746/files#r613660482. I can create a separate PR for that

ezyang · 2021-05-07T03:31:16Z

aten/src/ATen/native/LinearAlgebra.cpp

@ngimel I forgot... did I sign up to fix this in structured kernels itself 😂 🤣 (this is OK for now but shouldn't be necessary as structured kernel's set_output should do this test automatically)

We didn't figure out robust way of deduplicating size checks in structured kernel's set_output and TensorIterator right away, and I haven't looked at it yet in more detail.

ok filed an issue for this #57827

aten/src/ATen/native/LinearAlgebra.cpp

ezyang

Thanks, this isn't the cleanest port but it is probably about as good as it can get for now.

ngimel · 2021-05-07T16:37:15Z

Rong, can you please run instruction counts on it (on cpu and gpu)? addmm is perf sensitive, @swolchok spent a lot of time clamping down on its overhead, so would be bad to regress it.

facebook-github-bot · 2021-05-07T17:27:53Z

@walterddr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

walterddr · 2021-05-12T14:22:59Z

sorry for the late follow up: I used benchmarks/instruction_counts suite to count instructions on (20x40 x 40x10) on regular, in-place and out= versions. here is the instruction count differential before/after.

addmm_cpu      -0.29%
addmm_cpu_     -0.02%
addmm_cpu_out   0.01%
addmm_cuda     -0.86%
addmm_cuda_    -0.53%
addmm_cuda_out  0.15%

ngimel · 2021-05-12T16:05:19Z

Awesome, thanks!

- move one TORCH_CHECK back to impl since it is used in other mm funcs - skip LSTM/GRU meta test because it was reusing output then resized - added in-place checker for output

facebook-github-bot · 2021-05-12T21:26:08Z

@walterddr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2021-05-13T15:35:07Z

@walterddr merged this pull request in 002ce5c.

Summary: Port addmm to structure kernel Follow ups - migrate `mm` and `addbmm` to structure - move TORCH_CHECKS currently in `addmm_cpu_impl_` and `addmm_out_cuda_impl` to meta Pull Request resolved: pytorch#57417 Reviewed By: bdhirsh Differential Revision: D28291001 Pulled By: walterddr fbshipit-source-id: 4eafaa30a465e225fbb4d2a69a36f1e037df9122

Summary: relate to #57417. Pull Request resolved: #57755 Reviewed By: ezyang Differential Revision: D28426111 Pulled By: walterddr fbshipit-source-id: 943d3e36433ca846990b940177fb040553961156

swolchok · 2021-05-27T03:22:30Z

aten/src/ATen/native/LinearAlgebra.cpp

+
+  auto names = at::namedinference::propagate_names_for_addmm(mat1, mat2, self);
+  set_output(0, IntArrayRef({mat1.sizes()[0], mat2.sizes()[1]}), {}, self.options(), names);
+  auto result = maybe_get_output(0);


for future reference, this should have been const auto& to avoid a refcount bump -- maybe_get_output returns const Tensor&

facebook-github-bot added oncall: jit Add this issue/PR to JIT oncall triage queue cla signed labels May 1, 2021

walterddr removed the oncall: jit Add this issue/PR to JIT oncall triage queue label May 1, 2021

walterddr force-pushed the structure_kernel_addmm branch 3 times, most recently from c2ecf29 to 5f54466 Compare May 5, 2021 23:32

walterddr marked this pull request as ready for review May 6, 2021 15:34

walterddr requested a review from ezyang as a code owner May 6, 2021 15:34

imaginary-person mentioned this pull request May 6, 2021

Port addmm to structured #57713

Closed

ezyang reviewed May 7, 2021

View reviewed changes

walterddr mentioned this pull request May 7, 2021

port mm to structure kernel #57755

Closed

ezyang reviewed May 7, 2021

View reviewed changes

aten/src/ATen/native/LinearAlgebra.cpp Outdated Show resolved Hide resolved

walterddr mentioned this pull request May 7, 2021

Port kernels to be structured [tracker] #55070

Closed

ezyang approved these changes May 7, 2021

View reviewed changes

ezyang mentioned this pull request May 7, 2021

[structured] Inplace operations don't check outputs for illegal broadcasting of self in set_output #57827

Closed

walterddr force-pushed the structure_kernel_addmm branch from 5f54466 to 797fd04 Compare May 7, 2021 17:24

ngimel approved these changes May 12, 2021

View reviewed changes

Rong Rong added 6 commits May 12, 2021 10:00

[initial] commit for addmm structure kernel

c029ca2

fix a couple of TORCH_CHECKs

8c8f9d5

fix CI failures

d8a758b

- move one TORCH_CHECK back to impl since it is used in other mm funcs - skip LSTM/GRU meta test because it was reusing output then resized - added in-place checker for output

fix named tensor

4ac2d4b

also propagate name for mm

2d526f6

revert moving the expand_size to impl, there's no point

0a9d156

rebase and attempt to fix internal test failures

335025a

walterddr force-pushed the structure_kernel_addmm branch from 797fd04 to 335025a Compare May 12, 2021 21:12

facebook-github-bot closed this in 002ce5c May 13, 2021

facebook-github-bot added the Merged label May 13, 2021

bdhirsh changed the title ~~port addmm to structure kernel~~ addmm: port to structured kernel May 24, 2021

swolchok reviewed May 27, 2021

View reviewed changes

qingyunqu mentioned this pull request Jun 25, 2021

Port addbmm to structured kernels #60647

Closed

ngimel mentioned this pull request Jul 6, 2021

mm doesn't correctly check shape of GPU inputs #61291

Closed

ptrblck mentioned this pull request Jul 31, 2021

nn.Linear layer does not throw a shape mismatch error when using cuda devices #62539

Closed

This was referenced Aug 2, 2021

No input shape check on matmul (also nn.Linear) with CUDA backend #62562

Closed

nn.Linear does not give an error, even when the input matrix is too large or too small #62537

Closed

addmm: port to structured kernel #57417

addmm: port to structured kernel #57417

Uh oh!

Conversation

walterddr commented May 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot commented May 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💊 CI failures summary and remediations

Uh oh!

codecov bot commented May 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ezyang May 7, 2021

Choose a reason for hiding this comment

Uh oh!

walterddr May 7, 2021

Choose a reason for hiding this comment

Uh oh!

ezyang May 7, 2021

Choose a reason for hiding this comment

Uh oh!

ngimel May 7, 2021

Choose a reason for hiding this comment

Uh oh!

ezyang May 7, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ezyang left a comment

Choose a reason for hiding this comment

Uh oh!

ngimel commented May 7, 2021

Uh oh!

facebook-github-bot commented May 7, 2021

Uh oh!

walterddr commented May 12, 2021

Uh oh!

ngimel commented May 12, 2021

Uh oh!

facebook-github-bot commented May 12, 2021

Uh oh!

facebook-github-bot commented May 13, 2021

Uh oh!

swolchok May 27, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

`addmm`: port to structured kernel #57417

`addmm`: port to structured kernel #57417

walterddr commented May 1, 2021 •

edited

Loading

facebook-github-bot commented May 1, 2021 •

edited

Loading

codecov bot commented May 6, 2021 •

edited

Loading