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

[RISCV] Combine trunc (sra sext (x), zext (y)) to sra (x, smin (y, scalarsizeinbits(y) - 1)) #65728

Merged
merged 3 commits into from
Sep 17, 2023

Conversation

LWenH
Copy link
Member

@LWenH LWenH commented Sep 8, 2023

For RVV, If we want to perform an i8 or i16 element-wise vector arithmetic right shift in the upper C/C++ program, the value to be shifted would be first sign extended to i32, and the shift amount would also be zero_extended to i32 to perform the vsra.vv instruction, and followed by a truncate to get the final calculation result, such pattern will later expanded to a series of "vsetvli" and "vnsrl" instructions later, this is because the RVV spec only support 2 * SEW -> SEW truncate. But for vector, the shift amount can also be determined by smin (Y, ScalarSizeInBits(Y) - 1)). Also, for the vsra instruction, we only care about the low lg2(SEW) bits as the shift amount.

…NFC.

Add a series of pre-commit tests for later patch to perform
trunc (sra sext(X), zext(Y)) -> sra (X, smin (Y, scalarsize(Y) - 1))
combine.
…alarsize(Y) - 1))

For i8/i16 element-wise vector arithmetic right shift, the src value
would be first sign_extended to i32 and the shift amount would be
zero_extended to i32 to perform the vsra instruction, and followed
by a trunc to get the final calcualtion result. For RVV, the truncate
would be lowered into n-levels TRUNCATE_VECTOR_VL to satisfy RVV's SEW*2->SEW
truncate restriction, such pattern would be expanded into a series of "vsetvli"
and "vnsrl" instructions later. For RVV, we can use smin(Y, ScalarSizeInBits(Y)-1)
to determine the actual shift amount for the vsra instruction, because we only
care about the low lg2(SEW) bits as the shift amount.

For more transformation validation, please see alive2 links:
https://alive2.llvm.org/ce/z/wXLrLT
@LWenH LWenH requested a review from a team as a code owner September 8, 2023 09:06
@LWenH LWenH marked this pull request as draft September 8, 2023 09:11
@LWenH LWenH marked this pull request as ready for review September 8, 2023 09:17
@LWenH LWenH closed this Sep 8, 2023
@LWenH
Copy link
Member Author

LWenH commented Sep 8, 2023

How to add reviewers like in phabricator to request for review in github? I can't find that button in github interfaces(I tried that in the right side "Reviewers" button but that button is not clickable, may be that need some permission in github?).

@topperc @lukel97
Request for review, thank you.

@LWenH LWenH reopened this Sep 8, 2023
@LWenH LWenH changed the title [RISCV] Combine trunc (sra sext (X), zext (Y)) to sra (X, smin (Y, ScalarSizeInBits(Y) - 1)) [RISCV] Combine trunc (sra sext (x), zext (y)) to sra (x, smin (y, scalarsizeinbits(y) - 1)) Sep 8, 2023
@lukel97
Copy link
Contributor

lukel97 commented Sep 8, 2023

How to add reviewers like in phabricator to request for review in github? I can't find that button in github interfaces(I tried that in the right side "Reviewers" button but that button is not clickable, may be that need some permission in github?).

That's strange, clicking on the "Reviewers" label brings up this menu for me. I don't think you should need to be a member of the llvm-org since it's your own PR?

image

@LWenH
Copy link
Member Author

LWenH commented Sep 8, 2023

Yeah, I tried that one, but it still unclickable. I even tried to restart the pull request to invoke that lable, but it still doesn't work. BTW, thank you for helping me to add reviewers.

@lukel97
Copy link
Contributor

lukel97 commented Sep 8, 2023

No problem, I think you're right btw, it looks like only those with write access can request a review. I've flagged it in the discourse thread https://discourse.llvm.org/t/update-on-github-pull-requests/71540/105?u=lukel

@LWenH
Copy link
Member Author

LWenH commented Sep 15, 2023

Kindly Ping.

SDValue N10 = N1.getOperand(0);

if (N00.getValueType().isVector() &&
N00.getValueType() == N10.getValueType() && N->hasOneUse() &&
Copy link
Collaborator

@topperc topperc Sep 15, 2023

Choose a reason for hiding this comment

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

Why does N need to have a single use?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I agree with you. There is no need to judge hasOneUse for N here. Address comment.

@LWenH LWenH requested a review from topperc September 17, 2023 02:28
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

@LWenH
Copy link
Member Author

LWenH commented Sep 17, 2023

image
Hi, folks. I might still not have the write access to the github repository. Could reviewers merge this pull request for me please? BTW, I want to know how to get the write access for the github repository now?I didn't see the similar description in the new version of the github submission instructions. May be also need that write access to get the permission to add reviewers for pull requests in github right now?

Thank you for your time to read this question.

@dtcxzyw
Copy link
Member

dtcxzyw commented Sep 17, 2023

I will merge this pull request after this patch passes all regression tests on my machine (I cannot rerun the failed workflow on buildkite).
To obtain commit access, please refer to the developer policy:
https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access.

@LWenH
Copy link
Member Author

LWenH commented Sep 17, 2023

I will merge this pull request after this patch passes all regression tests on my machine (I cannot rerun the failed workflow on buildkite). To obtain commit access, please refer to the developer policy: https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access.

Thank you folks, that's a big help for me.

@dtcxzyw dtcxzyw merged commit ddae50d into llvm:main Sep 17, 2023
1 of 2 checks passed
ZijunZhaoCCK pushed a commit to ZijunZhaoCCK/llvm-project that referenced this pull request Sep 19, 2023
…alarsizeinbits(y) - 1)) (llvm#65728)

For RVV, If we want to perform an i8 or i16 element-wise vector
arithmetic right shift in the upper C/C++ program, the value to be
shifted would be first sign extended to i32, and the shift amount would
also be zero_extended to i32 to perform the vsra.vv instruction, and
followed by a truncate to get the final calculation result, such pattern
will later expanded to a series of "vsetvli" and "vnsrl" instructions
later, this is because the RVV spec only support 2 * SEW -> SEW
truncate. But for vector, the shift amount can also be determined by
smin (Y, ScalarSizeInBits(Y) - 1)). Also, for the vsra instruction, we
only care about the low lg2(SEW) bits as the shift amount.

- Alive2: https://alive2.llvm.org/ce/z/u3-Zdr
- C++ Test cases : https://gcc.godbolt.org/z/q1qE7fbha
fanghuaqi pushed a commit to riscv-mcu/llvm-project that referenced this pull request Oct 7, 2023
…alarsizeinbits(y) - 1)) (llvm#65728)

For RVV, If we want to perform an i8 or i16 element-wise vector
arithmetic right shift in the upper C/C++ program, the value to be
shifted would be first sign extended to i32, and the shift amount would
also be zero_extended to i32 to perform the vsra.vv instruction, and
followed by a truncate to get the final calculation result, such pattern
will later expanded to a series of "vsetvli" and "vnsrl" instructions
later, this is because the RVV spec only support 2 * SEW -> SEW
truncate. But for vector, the shift amount can also be determined by
smin (Y, ScalarSizeInBits(Y) - 1)). Also, for the vsra instruction, we
only care about the low lg2(SEW) bits as the shift amount.

- Alive2: https://alive2.llvm.org/ce/z/u3-Zdr
- C++ Test cases : https://gcc.godbolt.org/z/q1qE7fbha
LWenH added a commit to LWenH/llvm-project that referenced this pull request Oct 15, 2023
…alarsize(Y) - 1)

Like llvm#65728, for i8/i16 element-wise vector logical right shift,
the src value would be first zext to i32 and the shift amount
would be zext to i32 to perform the vsrl instruction, and followed
by a trunc to get the final calculation result. This would
be expanded into a series of "vsetvli" and "vnsrl" instructions
later. For RVV, the vsrl instruction only treats the lg2(sew)
bits as the shift amount, so we can calculate the shift amount
by using umin(Y,  scalarsize(Y) - 1).
zahiraam pushed a commit to tahonermann/llvm-project that referenced this pull request Oct 24, 2023
…alarsizeinbits(y) - 1)) (llvm#65728)

For RVV, If we want to perform an i8 or i16 element-wise vector
arithmetic right shift in the upper C/C++ program, the value to be
shifted would be first sign extended to i32, and the shift amount would
also be zero_extended to i32 to perform the vsra.vv instruction, and
followed by a truncate to get the final calculation result, such pattern
will later expanded to a series of "vsetvli" and "vnsrl" instructions
later, this is because the RVV spec only support 2 * SEW -> SEW
truncate. But for vector, the shift amount can also be determined by
smin (Y, ScalarSizeInBits(Y) - 1)). Also, for the vsra instruction, we
only care about the low lg2(SEW) bits as the shift amount.

- Alive2: https://alive2.llvm.org/ce/z/u3-Zdr
- C++ Test cases : https://gcc.godbolt.org/z/q1qE7fbha
zahiraam pushed a commit to tahonermann/llvm-project that referenced this pull request Oct 24, 2023
…alarsizeinbits(y) - 1)) (llvm#65728)

For RVV, If we want to perform an i8 or i16 element-wise vector
arithmetic right shift in the upper C/C++ program, the value to be
shifted would be first sign extended to i32, and the shift amount would
also be zero_extended to i32 to perform the vsra.vv instruction, and
followed by a truncate to get the final calculation result, such pattern
will later expanded to a series of "vsetvli" and "vnsrl" instructions
later, this is because the RVV spec only support 2 * SEW -> SEW
truncate. But for vector, the shift amount can also be determined by
smin (Y, ScalarSizeInBits(Y) - 1)). Also, for the vsra instruction, we
only care about the low lg2(SEW) bits as the shift amount.

- Alive2: https://alive2.llvm.org/ce/z/u3-Zdr
- C++ Test cases : https://gcc.godbolt.org/z/q1qE7fbha
@LWenH LWenH deleted the fixvsra branch October 31, 2023 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants