Skip to content

[tmva][sofie] fix double-counted dilation in Conv im2col and adding regression test#22474

Open
harz05 wants to merge 1 commit into
root-project:masterfrom
harz05:fix/conv-dilation
Open

[tmva][sofie] fix double-counted dilation in Conv im2col and adding regression test#22474
harz05 wants to merge 1 commit into
root-project:masterfrom
harz05:fix/conv-dilation

Conversation

@harz05
Copy link
Copy Markdown
Contributor

@harz05 harz05 commented Jun 3, 2026

Changes or fixes:

Conv generated invalid code and crashed (segfault) for dilation > 1. The operator builds a dilated _f weight buffer and expands fAttrKernelShape to the effective kernel size, but the im2col call was still passed the real dilation, so dilation was applied twice (expanded kernel + dilation). This produced a negative output dimension and an out-of-bounds write.

Fix: after the dilated _f is built, the dense im2col must use dilation 1, since the dilation is already folded into the expanded kernel shape and the _f layout. This is a no-op for dilation 1, so existing Conv tests are unaffected.

Also adds a ConvWithDilation test (3x3 kernel, dilation 2) with reference output. The suite had no dilation > 1 case before, which is why this was never caught. The full SOFIE test suite passes locally on master.

Checklist:

  • tested changes locally
  • updated the docs (if necessary)

This PR fixes #22473

@harz05 harz05 requested a review from lmoneta as a code owner June 3, 2026 17:47
@harz05
Copy link
Copy Markdown
Contributor Author

harz05 commented Jun 3, 2026

kindly review @guitargeek ,thanks

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jun 4, 2026

Test Results

    22 files      22 suites   3d 10h 2m 9s ⏱️
 3 853 tests  3 853 ✅ 0 💤 0 ❌
76 127 runs  76 127 ✅ 0 💤 0 ❌

Results for commit a6c3952.

♻️ This comment has been updated with latest results.

@harz05
Copy link
Copy Markdown
Contributor Author

harz05 commented Jun 4, 2026

can we try a clean build once? @guitargeek

@ferdymercury ferdymercury added the clean build Ask CI to do non-incremental build on PR label Jun 4, 2026
@ferdymercury ferdymercury reopened this Jun 4, 2026
@ferdymercury ferdymercury requested a review from guitargeek June 4, 2026 07:32
@harz05
Copy link
Copy Markdown
Contributor Author

harz05 commented Jun 4, 2026

tests are passing, only clang formatting is failing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

clean build Ask CI to do non-incremental build on PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[tmva][sofie] Conv generates invalid code and crashes for dilation > 1

2 participants