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

[SDPatternMatch] Do not use std::forward and rvalue references (NFC) #93806

Merged
merged 1 commit into from
Jun 6, 2024

Conversation

nikic
Copy link
Contributor

@nikic nikic commented May 30, 2024

The m_ZExtOrSelf() family of matchers currently incorrectly calls std::forward twice on the same value. However, just removing those causes other complications, because then template arguments get incorrectly inferred to const references instead of the underlying value types. Things become a mess.

Instead, just completely remove the use of std::forward and rvalue references from SDPatternMatch. I don't think they really provide value in this context, especially as they're not used consistently in the first place.

@nikic nikic requested review from RKSimon and mshockwave May 30, 2024 11:19
Copy link

github-actions bot commented May 30, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

The m_ZExtOrSelf() family of matchers currently incorrect calls
std::forward twice on the value. However, just removing those causes
other complications, because then template arguments get incorrectly
inferred to const references instead of the underlying value types.
Things become a mess.

Instead, just completely remove the use of std::forward and
rvalue references from SDPatternMatch, aligning it with normal IR
PatternMatch.
@nikic nikic force-pushed the sd-pattern-match-forward branch from 0a43d07 to 2c6aa67 Compare May 30, 2024 11:25
Copy link
Collaborator

@RKSimon RKSimon left a comment

Choose a reason for hiding this comment

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

LGTM - unless @mshockwave had a future plan for this?

Copy link
Member

@mshockwave mshockwave left a comment

Choose a reason for hiding this comment

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

LGTM.
Yeah I also realized the sub-pattern template types in SDPatternMatch::And and SDPatternMatch::Or are kind of off. Thanks for fixing them.

@nikic nikic merged commit afc7292 into llvm:main Jun 6, 2024
7 checks passed
@nikic nikic deleted the sd-pattern-match-forward branch June 6, 2024 07:59
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.

3 participants