-
Notifications
You must be signed in to change notification settings - Fork 151
simplify and fix blasDiffUse #1332
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
Conversation
|
I think the cache part shouldn't be here since diffuse analysis is used by the algorithm to determine caching. |
|
cacheMode just checks if it's forwardModeonly. So it's only the name, or am I using any information that I should not? |
| os << " if (active_" << name << ") return true;\n"; | ||
| } | ||
| os << " }\n"; | ||
| os << " if (val == arg_" << name << " && need_" << name << " && !cache_" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like mentioned on slack, I don't think it's right to do cache checks here since this analysis is used by cache analysis.
Can you revert the cache change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah sorry I meant on here, not slack.
But yeah I believe there's an effective circular dependency, which here will be manifested by inconsistent results (not a compile time error), which is potentially very bad.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just to clarify @wsmoses. If it existend then it existed just on the same level before this PR. I did use exactly the same logic before this change, I just re-did some of the calculation instead of reusing the variables like I did now. If you want this to be fundamentally changed reverting will not change it, we would rather need to see what else tk use
Apparently you can introduce subtle bugs if you re-do work unnecessarily.
@wsmoses this new behavior should be always correct, right?
At least it will fix the lda issue for runtime-activity. Adding some more tests there rn.