Skip to content

Comments

Skip col_buffer allocation when not needed#1171

Closed
sguada wants to merge 3 commits intoBVLC:devfrom
sguada:skip_col_buff
Closed

Skip col_buffer allocation when not needed#1171
sguada wants to merge 3 commits intoBVLC:devfrom
sguada:skip_col_buff

Conversation

@sguada
Copy link
Contributor

@sguada sguada commented Sep 27, 2014

In the line of #1118 this PR based on the discussion with @longjon and @shelhamer extends the cases when the col_buffer is not needed and a matrix multiplication is enough to compute the result.

@longjon
Copy link
Contributor

longjon commented Sep 27, 2014

Thanks for PRing this. However, see my comment #1118 (comment); I believe your expression is wrong and breaks convolution in certain cases.

Also, I think we should test this rather more aggressively in terms of the parameter space than the existing conv tests (even if we only do this once, offline) as in any case this will add significant subtlety to the code paths.

@sguada
Copy link
Contributor Author

sguada commented Sep 27, 2014

@longjon agreed, we need a more complete set of tests. Currently I only need the first two cases 1x1 and Single, for which I have added test cases, so we can keep that two in this PR and leave the other 2 for later, or I add them now and test them offline (I think they a less frequent use case).

(pad_h == 0 && pad_w == 0) && 
  ((kernel_w_ == 1 && kernel_h_ == 1 && stride_h_ == 1 && stride_w_ == 1) ||  // 1x1
  (width == kernel_w && height == kernel_h) ||  // Single
  (stride_w == kernel_w && width % kernel_w == 0 && kernel_h == 1) ||
  (width == kernel_w && stride_h == kernel_h && height % kernel_h == 0))

@longjon
Copy link
Contributor

longjon commented Sep 28, 2014

(See #1118 (comment), and let's continue the discussion here.)

If it's not too onerous to gain confidence in its correctness, we might as well use the general expression now.

Since all the tests are for !skip_col_buff_, perhaps we should compute need_col_buff_ instead?

@sguada
Copy link
Contributor Author

sguada commented Dec 19, 2014

@longjon could we merge this?

@sguada
Copy link
Contributor Author

sguada commented Dec 26, 2014

@longjon although we could compute need_col_buff_ I think the logic is easier to follow with skip_col_buff_.
The first two cases are clear and very useful, so if you are not comfortable with the last two cases let's merge the first two.

@shelhamer
Copy link
Member

Closing since the dev branch is deprecated. Please send PRs to master.

@shelhamer shelhamer closed this Aug 26, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants