Skip to content

[SDPatternMatch] Only match ISD::SIGN_EXTEND in m_SExt #95415

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 1 commit into from
Jun 14, 2024

Conversation

c8ef
Copy link
Contributor

@c8ef c8ef commented Jun 13, 2024

Context: #95365 (comment)

The current implementation of m_SExt matches both ISD::SIGN_EXTEND and ISD::SIGN_EXTEND_INREG. However, in cases where we specifically need to match only ISD::SIGN_EXTEND, such as in the SelectionDAG graph below, this can lead to issues and unintended combinations.

SelectionDAG has 13 nodes:
  t0: ch,glue = EntryToken
          t2: v2i32,ch = CopyFromReg t0, Register:v2i32 %0
        t21: v2i32 = sign_extend_inreg t2, ValueType:ch:v2i8
          t4: v2i32,ch = CopyFromReg t0, Register:v2i32 %1
        t22: v2i32 = sign_extend_inreg t4, ValueType:ch:v2i8
      t23: v2i32 = avgfloors t21, t22
    t24: v2i32 = sign_extend_inreg t23, ValueType:ch:v2i8
  t15: ch,glue = CopyToReg t0, Register:v2i32 $d0, t24
  t16: ch = AArch64ISD::RET_GLUE t15, Register:v2i32 $d0, t15:1

@RKSimon
Copy link
Collaborator

RKSimon commented Jun 13, 2024

Cleanup the PR title and summary. Title should be something like "[SDPatternMatch] Only match ISD::SIGN_EXTEND in m_SExt" and then refer to #90762 in the summary and explain why we need this.

@RKSimon RKSimon requested a review from topperc June 13, 2024 15:56
@c8ef c8ef changed the title [SDPatternMatch] revert SExt changes in #90762 [SDPatternMatch] Only match ISD::SIGN_EXTEND in m_SExt Jun 14, 2024
@c8ef
Copy link
Contributor Author

c8ef commented Jun 14, 2024

Cleanup the PR title and summary. Title should be something like "[SDPatternMatch] Only match ISD::SIGN_EXTEND in m_SExt" and then refer to #90762 in the summary and explain why we need this.

Fixed. I will be more careful with the PR description and title in the future.

Copy link
Collaborator

@topperc topperc left a comment

Choose a reason for hiding this comment

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

LGTM

@RKSimon RKSimon merged commit 4f54b91 into llvm:main Jun 14, 2024
8 checks passed
@c8ef c8ef deleted the sext-match branch June 14, 2024 09:54
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