[RISCV] Fold (add (srl x, n), (srl x, n)) into (srl x, n-1) #144507
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This patch adds a new fold that will turn (add (srl x, n), (srl x, n)) into (srl x, n-1) when bit n-1 is known to be zero in x. This could perhaps be moved to generic DAGCombiner in the future, but this patch adds it as a RISCV specific combine. For RISCV it typically trigger for DAG nodes like this that may be created by the legalizer: t1: i32 = srl RISCVISD::READ_VLENB:i32, Constant:i32<2> t2: i32 = add t1, t1 Got the idea when working on a solution for #141034, as it may avoid some regressions otherwise caused by the fix being prepared for that issue.
topperc
reviewed
Jun 17, 2025
| @@ -15240,12 +15240,42 @@ static SDValue combineAddOfBooleanXor(SDNode *N, SelectionDAG &DAG) { | |||
| N0.getOperand(0)); | |||
| } | |||
|
|
|||
| // Try to turn (add (srl x, n), (srl x, n)) into (srl x, n-1). | |||
| // | |||
| // This combine could perhaps be moved to DAGCombiner. For RISCV this kind of | |||
There was a problem hiding this comment.
RISCV should be spelled RISC-V in comments. It's a trademark.
|
I may have an alternate proposal for this patch. Investigating. |
topperc
added a commit
to topperc/llvm-project
that referenced
this pull request
Jun 17, 2025
… (C0 + C1)) in getNode. This is an alternative to the DAGCombine proposed in llvm#144507.
My alternate proposal #144565 which covers the READ_VLENB cases. |
Thanks! That seems like a better solution. I'll abandon this. |
fschlimb
pushed a commit
to fschlimb/llvm-project
that referenced
this pull request
Jun 18, 2025
…ale * (C0 + C1)) in getNode. (llvm#144565) We already have shl/mul vscale related folds in getNode. This is an alternative to the DAGCombine proposed in llvm#144507.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch adds a new fold that will turn
(add (srl x, n), (srl x, n))
into
(srl x, n-1)
when bit n-1 is known to be zero in x.
This could perhaps be moved to generic DAGCombiner in the future, but this patch adds it as a RISCV specific combine. For RISCV it typically trigger for DAG nodes like this that may be created by the legalizer:
t1: i32 = srl RISCVISD::READ_VLENB:i32, Constant:i32<2>
t2: i32 = add t1, t1
Got the idea when working on a solution for #141034, as it may avoid some regressions otherwise caused by the fix being prepared for that issue.