Skip to content

Add debug asserts #1566

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Nov 20, 2018
Merged

Add debug asserts #1566

merged 2 commits into from
Nov 20, 2018

Conversation

jwood803
Copy link
Contributor

@jwood803 jwood803 commented Nov 7, 2018

Potential fix for #828

@briancylui Is this on the right track to what was needed? Do you think we should do the same to the AvxIntrinsics class, as well?

@TomFinley
Copy link
Contributor

Hi @jwood803 , this approach looks good to me. Thank you for working on it. I'm not certain if @briancylui is still looking at this (he's quite welcome to, just don't think he is). You mention this is a WIP so I imagine there may be more to come, so I will not formally approve just yet.

For those methods that take AlignedArray, we might consider that also having a Contracts.AssertValue on the inputs would also be valuable (aligned array is a reference type), but I do not view this as in any way essential or required given that the ultimate goal would be (this pursuant to #1028 and likely followup work).

@jwood803 jwood803 changed the title [WIP] Add debug asserts Add debug asserts Nov 8, 2018
Copy link
Contributor

@Zruty0 Zruty0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@Zruty0 Zruty0 merged commit 3ce839e into dotnet:master Nov 20, 2018
@Anipik
Copy link
Contributor

Anipik commented Nov 20, 2018

@Zruty0 @danmosemsft can we revert this change ? some of the debug assumptions are not true. eg in matrix multplivation destionation and src length need not be same
And Currently Contracts.Assert() are being removed from netcoreapp3.0 build.

@jwood803 jwood803 deleted the add-assert branch November 20, 2018 11:24
@jwood803
Copy link
Contributor Author

@Anipik @Zruty0 I can make additional PRs for this. One to update the asserts and another to move away from Contracts.Assert.

@Anipik
Copy link
Contributor

Anipik commented Nov 20, 2018

@jwood803 if you want to correct the change, just remove the contracts.Assert statements from the matMul, matMulTrans and matmulP in a PR ?

@ghost ghost locked as resolved and limited conversation to collaborators Mar 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants