[GISel] Funnel shift combiner port from SelectionDAG ISel to GlobalISel #135132
Conversation
|
@mshockwave I finished that combiner port! Let me know what you think :) |
|
@llvm/pr-subscribers-backend-risc-v @llvm/pr-subscribers-llvm-globalisel Author: Axel Sorenson (axelcool1234) ChangesThe funnel shift combiner rule from 4a3708c is currently missing from GlobalISel. The following is a port of that combiner to GlobalISel. If anyone has a better name for the tablegen functions (they're currently called Full diff: https://github.com/llvm/llvm-project/pull/135132.diff 2 Files Affected:
diff --git a/llvm/include/llvm/Target/GlobalISel/Combine.td b/llvm/include/llvm/Target/GlobalISel/Combine.td
index 5309d5952f087..615617d4ca01b 100644
--- a/llvm/include/llvm/Target/GlobalISel/Combine.td
+++ b/llvm/include/llvm/Target/GlobalISel/Combine.td
@@ -1033,6 +1033,24 @@ def funnel_shift_overshift: GICombineRule<
(apply [{ Helper.applyFunnelShiftConstantModulo(*${root}); }])
>;
+// Transform: fshl x, ?, y | shl x, y -> fshl x, ?, y
+def funnel_shift_or_shift_to_funnel_shift_left: GICombineRule<
+ (defs root:$root),
+ (match (G_FSHL $out1, $x, $_, $y),
+ (G_SHL $out2, $x, $y),
+ (G_OR $root, $out1, $out2)),
+ (apply (G_FSHL $root, $x, $_, $y))
+>;
+
+// Transform: fshr ?, x, y | srl x, y -> fshr ?, x, y
+def funnel_shift_or_shift_to_funnel_shift_right: GICombineRule<
+ (defs root:$root),
+ (match (G_FSHR $out1, $_, $x, $y),
+ (G_LSHR $out2, $x, $y),
+ (G_OR $root, $out1, $out2)),
+ (apply (G_FSHR $root, $_, $x, $y))
+>;
+
def rotate_out_of_range : GICombineRule<
(defs root:$root),
(match (wip_match_opcode G_ROTR, G_ROTL):$root,
@@ -1105,7 +1123,9 @@ def funnel_shift_combines : GICombineGroup<[funnel_shift_from_or_shift,
funnel_shift_to_rotate,
funnel_shift_right_zero,
funnel_shift_left_zero,
- funnel_shift_overshift]>;
+ funnel_shift_overshift,
+ funnel_shift_or_shift_to_funnel_shift_left,
+ funnel_shift_or_shift_to_funnel_shift_right]>;
def bitfield_extract_from_sext_inreg : GICombineRule<
(defs root:$root, build_fn_matchinfo:$info),
diff --git a/llvm/test/CodeGen/RISCV/GlobalISel/shift.ll b/llvm/test/CodeGen/RISCV/GlobalISel/shift.ll
index 75e318a58fd45..b52924cac0031 100644
--- a/llvm/test/CodeGen/RISCV/GlobalISel/shift.ll
+++ b/llvm/test/CodeGen/RISCV/GlobalISel/shift.ll
@@ -105,3 +105,55 @@ define i16 @test_shl_i48_2(i48 %x, i48 %y) {
%trunc = trunc i48 %shl to i16
ret i16 %trunc
}
+
+define i16 @test_fshl_i32(i32 %x, i32 %_, i32 %y) {
+; RV32-LABEL: test_fshl_i32:
+; RV32: # %bb.0:
+; RV32-NEXT: not a3, a2
+; RV32-NEXT: sll a0, a0, a2
+; RV32-NEXT: srli a1, a1, 1
+; RV32-NEXT: srl a1, a1, a3
+; RV32-NEXT: or a0, a0, a1
+; RV32-NEXT: ret
+;
+; RV64-LABEL: test_fshl_i32:
+; RV64: # %bb.0:
+; RV64-NEXT: not a3, a2
+; RV64-NEXT: sllw a0, a0, a2
+; RV64-NEXT: srliw a1, a1, 1
+; RV64-NEXT: srlw a1, a1, a3
+; RV64-NEXT: or a0, a0, a1
+; RV64-NEXT: ret
+
+ %fshl = call i32 @llvm.fshl.i32(i32 %x, i32 %_, i32 %y)
+ %shl = shl i32 %x, %y
+ %or = or i32 %fshl, %shl
+ %trunc = trunc i32 %or to i16
+ ret i16 %trunc
+}
+
+define i16 @test_fshr_i32(i32 %_, i32 %x, i32 %y) {
+; RV32-LABEL: test_fshr_i32:
+; RV32: # %bb.0:
+; RV32-NEXT: not a3, a2
+; RV32-NEXT: slli a0, a0, 1
+; RV32-NEXT: sll a0, a0, a3
+; RV32-NEXT: srl a1, a1, a2
+; RV32-NEXT: or a0, a0, a1
+; RV32-NEXT: ret
+;
+; RV64-LABEL: test_fshr_i32:
+; RV64: # %bb.0:
+; RV64-NEXT: not a3, a2
+; RV64-NEXT: slli a0, a0, 1
+; RV64-NEXT: sllw a0, a0, a3
+; RV64-NEXT: srlw a1, a1, a2
+; RV64-NEXT: or a0, a0, a1
+; RV64-NEXT: ret
+
+ %fshr = call i32 @llvm.fshr.i32(i32 %_, i32 %x, i32 %y)
+ %lshr = lshr i32 %x, %y
+ %or = or i32 %fshr, %lshr
+ %trunc = trunc i32 %or to i16
+ ret i16 %trunc
+}
|
|
Can you regenerate llvm/test/CodeGen/AArch64/funnel-shift.ll too? It looks like it is failing in the precommit tests and needs an update. |
axelcool1234
force-pushed
the
fsh-Combiner-Port
branch
from
e8b1457 to
6b8c8bd
Compare
|
I updated the AArch64 test using |
davemgreen
left a comment
davemgreen
left a comment
There was a problem hiding this comment.
Thanks. The combine looks good - nice and clean - other than the points mentioned below.
We would not usually need to add combines that the midend can reliably perform unless they come up during lowering. I am not sure if that is expected in this case or not, from the legalization of higher sized types or from vectors.
axelcool1234
force-pushed
the
fsh-Combiner-Port
branch
2 times, most recently
from
17c74e0 to
733135e
Compare
There was a problem hiding this comment.
Please refrain from force push unless it's necessary: https://www.llvm.org/docs/GitHub.html#rebasing-pull-requests-and-force-pushes.
Please append new commits instead, as they're all gonna be squash into one at the end.
cdevadas
changed the title
|
I don't see why the hasOneUse checks are needed, and the DAG version that you copied does not have them. |
I was told by @arsenm that it was a common combiner bug to lack the check. However, I do wonder: I think my confusion stems from what happens when we do
If that's correct, then I think @jayfoad might be right: if the |
The combine doesn't really remove the original instructions, it just replaces all uses of the original root instruction. Then it's up to the normal dead code elimination to remove the original instructions if they are unused. The point of a hasOneUse check is to make sure that one of the sub-instructions in the pattern will be unused, because it doesn't have any other uses outside of the pattern we matched. Typically for a cmobine that creates a new instruction, you would use hasOneUse checks, because you don't want to create a new instruction unless you can remove one or more of the existing instructions. But this combine does not create any new instructions, so I don't think there's any particular need for tha hasOneUse check. |
|
Now that the Additionally, this file lacks vector versions of any of the shift tests. I'll look into introducing them. |
According to the docs, RISC-V supports vectory types: https://llvm.org/docs/RISCV/RISCVVectorExtension.html. However, every time I try to write a trivial function test with a vector I get an error stating that RISC-V is unable to "lower" the arguments. I think the reason I can't use them has to do with the test running a version of RISC-V without the vector extension enabled. What should I change the |
To enable vectors, you can |
|
@mshockwave So I've been exposed to scalable vectors for the first time - I've been exploring how they work in RISC-V (its very interesting). Please tell me if I'm mistaken, but it seems GlobalISel only supports scalable vectors (and not fixed vectors) for arguments to functions. Despite this, it seems most scalable vector instructions are not supported in GlobalISel. All of my fiddling around with LLVM IR is getting me If this is all true, how should I construct these vector tests? Since I can't seem to pass fixed length vectors as arguments, I'm forced to write them in the function body (but this leads to the compiler constant folding it all down since it statically knows the content of said fixed vectors). Unless there's a better way, perhaps I should look into loading the vector from a pointer or something? Admittedly, I feel I haven't done enough digging or deeper reading into this (beyond the language reference and this very useful article, combined with your blog post on this). If you have any resources I should really sink my teeth into, I will do so. Thanks again! |
|
@mshockwave it's been a while (I personally got busy), but I'm still interested in getting this merged. It's basically ready (beyond needing to rebase to the most recent commits of course), I just need some input on the vector tests another reviewer mentioned. The original SelectionDAG ISel version of this didn't have any, and the RISCV backend of GlobalISel is making it tricky to write vector tests. Do I even need them here? If so, can you read my previous comment and let me know how I should proceed? I want to thank you again for taking the time to help me get involved in the LLVM codebase. |
Oh nice! I'm glad you're still keen to work on this. Sorry I saw your previous comment but I was pretty occupied back then. So personally I think you could go ahead with only the scalar tests. Please rebase your PR and I'll give another round of review tomorrow morning. Thanks again for pushing this forward! |
Awesome, I'll make sure to rebase tomorrow. I'd like to email you too about continuing my contributions to LLVM (i.e. what I should explore/do next) under your guidance - I know there's been a 3 month gap but I'd love to keep going if you're up for it. |
yeah absolutely |
The funnel shift combiner rule from 4a3708c is currently missing from GlobalISel. The following is a port of that combiner to GlobalISel.
axelcool1234
force-pushed
the
fsh-Combiner-Port
branch
from
275c274 to
75ad0e0
Compare
|
@mshockwave I have made a pre-commit test and a post-commit with the PR's title and description. Let me know if this is correct! And if so, then this can be merged :) |
It's merged now. For future reference: you don't have to squash commits manually because Github would do that for you upon merging ("Squash and Merge" is the only option for LLVM at this moment). The pre-commit test system is also just for reviewing purposes (reviewers could navigate between different commits to see the difference), you don't really need to split it out for merging as they'll all gonna squashed into one at the end. |
6 participants
The funnel shift combiner rule from 4a3708c is currently missing from GlobalISel. The following is a port of that combiner to GlobalISel.