Skip to content
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

[TIR] More hygenic TVM_SREF macros #12607

Merged
merged 2 commits into from
Aug 26, 2022
Merged

Conversation

Lunderberg
Copy link
Contributor

@Lunderberg Lunderberg commented Aug 25, 2022

Previously, the TVM_SREF_TO_BLOCK, TVM_SREF_TO_FOR, andTVM_TYPE_AS macros required both the input and output variables. The input variable name is useful for improving the error message returned, but the output variable name isn't necessary for this functionality, and prevents the macro from being used as part of an expression.

  • Generate an immediately-invoked lambda expression to allow for an independently-scoped result variable.

  • Use parentheses around the input argument, in case the sref is the result of an expression.

  • Update all call sites to remove the macro argument providing the first argument.

cc @Hzfengsy @junrushao1994

Previously, the `TVM_SREF_TO_BLOCK`, `TVM_SREF_TO_FOR`, and
`TVM_TYPE_AS` macros required both the input and output variables.
The input variable name is useful for improving the error message
returned, but the output variable name isn't necessary for this
functionality, and prevents the macro from being used as part of an
expression.

* Generate an immediately-invoked lambda expression to allow for an
  independently-scoped `result` variable.

* Use parentheses around the input argument, in case the sref is
  the result of an expression.

* Update all call sites to remove the macro argument providing the
  first argument.
@Lunderberg
Copy link
Contributor Author

This is currently opened as a draft PR, to see if there are breakages that weren't caught, and whether there should be a rollover from the output-parameter version to the new version. It's only an internal macro, but it could cause additional merge conflicts with existing PRs.

@junrushao
Copy link
Member

I think it's a perfect solution actually :-)

@junrushao
Copy link
Member

The only reason that I didn't do so previously was being a bit afraid that the lambda cannot be inlined...CC: @tqchen

@Lunderberg
Copy link
Contributor Author

Cool, cool! I wasn't sure if there were other benefits to the macro that I had been missing, and wanted to be careful about breakage when tromping on a less familiar part of the codebase.

Regarding inlining, my understanding is that lambdas can always be inlined by the as-if rule, and their limited scope usually lets the compiler be more aggressive about inlining than for normal functions. I hadn't ever verified this from the assembly before, but a couple of quick godbolt comparisons (function call not marked inline, function call marked inline, immediately invoked lambda) confirm that lambdas can be inlined with the same gcc version/optimization settings as are used on TVM's CI.

@Lunderberg Lunderberg marked this pull request as ready for review August 26, 2022 13:17
Copy link
Member

@Hzfengsy Hzfengsy left a comment

Choose a reason for hiding this comment

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

Thanks for improving the developer experience. The new MACRO looks much clearer than the current one

@vinx13 vinx13 merged commit 49b3c72 into apache:main Aug 26, 2022
@Lunderberg Lunderberg deleted the tvm_sref_macro branch August 26, 2022 19:26
xinetzone pushed a commit to daobook/tvm that referenced this pull request Nov 25, 2022
Previously, the `TVM_SREF_TO_BLOCK`, `TVM_SREF_TO_FOR`, and
`TVM_TYPE_AS` macros required both the input and output variables.
The input variable name is useful for improving the error message
returned, but the output variable name isn't necessary for this
functionality, and prevents the macro from being used as part of an
expression.

* Generate an immediately-invoked lambda expression to allow for an
  independently-scoped `result` variable.

* Use parentheses around the input argument, in case the sref is
  the result of an expression.

* Update all call sites to remove the macro argument providing the
  first argument.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants