-
Notifications
You must be signed in to change notification settings - Fork 79
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
transformations: (lower-csl-wrapper) Add params_as_consts flag #3324
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3324 +/- ##
=======================================
Coverage 90.08% 90.08%
=======================================
Files 446 446
Lines 56419 56422 +3
Branches 5417 5419 +2
=======================================
+ Hits 50824 50829 +5
+ Misses 4162 4161 -1
+ Partials 1433 1432 -1 ☔ View full report in Codecov by Sentry. |
The PR does not indicate why this flag is needed and also lacks a testcase. Either add a test case or explain in the commit message why no test case can be added. |
// CONST-NEXT: "csl.rpc"(%0) : (!csl.color) -> () | ||
// CONST-NEXT: }) {"sym_name" = "gauss_seidel_func_program"} : () -> () | ||
// CONST-NEXT: } | ||
// CONST-EMPTY: |
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.
This test case is huge. Do we need such a large test case to test this feature? Or can there be a smaller minimal test case?
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.
I added a shorter test, with and without the flag. @dk949
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.
looks good to me!
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.
Besides my one comment, this looks great.
…roject#3324) CSL module wrapper params are by default lowered to CSL params, which can have a value specified in code that can be overridden manually by providing command line inputs. If this is not desired, module wrapper params can be lowered to constants instead by setting the `params_as_consts` flag. For this to happen, the module wrapper params must be numerical and have a specified default value. Use this flag for programs if the generated program does not support arbitrary param values. --------- Co-authored-by: n-io <n-io@users.noreply.github.com>
CSL module wrapper params are by default lowered to CSL params, which can have a value specified in code that can be overridden manually by providing command line inputs. If this is not desired, module wrapper params can be lowered to constants instead by setting the
params_as_consts
flag. For this to happen, the module wrapper params must be numerical and have a specified default value.Use this flag for programs if the generated program does not support arbitrary param values.