[DAG] Remove OneUse restriction on sext when folding (shl (sext (add_nsw x, c1)), c2) #68972

LiqinWeng · 2023-10-13T10:24:33Z

This patch remove the restriction for folding (shl (sext (add_nsw x, c1)), c2) -> (add (shl (sext x), c2), c1 << c2), and test case from dhrystone , see this link: https://godbolt.org/z/8zfa3rnad

llvmbot · 2023-10-13T10:25:48Z

@llvm/pr-subscribers-backend-x86
@llvm/pr-subscribers-backend-risc-v

@llvm/pr-subscribers-llvm-selectiondag

Author: LiqinWeng (LiqinWeng)

Changes

This patch remove the restriction for folding (shl (sext (add_nsw x, c1)), c2) -> (add (shl (sext x), c2), c1 << c2), and test case from dhrystone , see this link: https://godbolt.org/z/5KnjK16oG

Full diff: https://github.com/llvm/llvm-project/pull/68972.diff

2 Files Affected:

(modified) llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp (-1)
(added) llvm/test/CodeGen/RISCV/riscv-shifted-extend.ll (+29)

diff --git a/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp b/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
index 1021b07da1ac6c5..383edd07db59c94 100644
--- a/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
@@ -10046,7 +10046,6 @@ SDValue DAGCombiner::visitSHL(SDNode *N) {
   if (N0.getOpcode() == ISD::SIGN_EXTEND &&
       N0.getOperand(0).getOpcode() == ISD::ADD &&
       N0.getOperand(0)->getFlags().hasNoSignedWrap() && N0->hasOneUse() &&
-      N0.getOperand(0)->hasOneUse() &&
       TLI.isDesirableToCommuteWithShift(N, Level)) {
     SDValue Add = N0.getOperand(0);
     SDLoc DL(N0);
diff --git a/llvm/test/CodeGen/RISCV/riscv-shifted-extend.ll b/llvm/test/CodeGen/RISCV/riscv-shifted-extend.ll
new file mode 100644
index 000000000000000..ccb57b45e563fd0
--- /dev/null
+++ b/llvm/test/CodeGen/RISCV/riscv-shifted-extend.ll
@@ -0,0 +1,29 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 3
+; RUN: llc -mtriple=riscv64 -verify-machineinstrs < %s \
+; RUN:   | FileCheck -check-prefix=RV64 %s
+
+define dso_local void @test(ptr nocapture noundef writeonly %array1, i32 noundef signext %a, i32 noundef signext %b) local_unnamed_addr #0 {
+; RV64-LABEL: test:
+; RV64:       # %bb.0: # %entry
+; RV64-NEXT:    addi a3, a1, 5
+; RV64-NEXT:    slli a1, a1, 2
+; RV64-NEXT:    add a0, a1, a0
+; RV64-NEXT:    sw a2, 20(a0)
+; RV64-NEXT:    sw a2, 24(a0)
+; RV64-NEXT:    sw a3, 140(a0)
+; RV64-NEXT:    ret
+entry:
+  %add = add nsw i32 %a, 5
+  %idxprom = sext i32 %add to i64
+  %arrayidx = getelementptr inbounds i32, ptr %array1, i64 %idxprom
+  store i32 %b, ptr %arrayidx, align 4
+  %add3 = add nsw i32 %a, 6
+  %idxprom4 = sext i32 %add3 to i64
+  %arrayidx5 = getelementptr inbounds i32, ptr %array1, i64 %idxprom4
+  store i32 %b, ptr %arrayidx5, align 4
+  %add6 = add nsw i32 %a, 35
+  %idxprom7 = sext i32 %add6 to i64
+  %arrayidx8 = getelementptr inbounds i32, ptr %array1, i64 %idxprom7
+  store i32 %add, ptr %arrayidx8, align 4
+  ret void
+}

goldsteinn · 2023-10-13T17:12:54Z

Can you split the new tests into a seperate commit s.t we can see the diff this patch generates.
"Remove restrictions" is a bit broad of a title for a patch only concerned with (shl (sext (add_nsw x, c1)), c2). Maybe something along the lines of "Remove OneUse restriction on sext when folding (shl (sext (add_nsw x, c1)), c2)"?

goldsteinn · 2023-10-13T17:13:17Z

llvm/test/CodeGen/X86/pr65895.ll

 ; CHECK-NEXT:    addb $-3, %al
 ; CHECK-NEXT:    movsbl %al, %eax
 ; CHECK-NEXT:    movl %eax, d(%rip)
-; CHECK-NEXT:    leal 247(%rax,%rax,2), %eax
+; CHECK-NEXT:    leal 241(%rax,%rcx,2), %eax


This is an objective regression.

Regression still needs fixing

goldsteinn · 2023-10-14T15:41:54Z

llvm/test/CodeGen/X86/pr65895.ll

@@ -21,10 +21,11 @@ define i32 @PR65895() {
 ; CHECK-NEXT:    # =>This Inner Loop Header: Depth=1
 ; CHECK-NEXT:    jmp .LBB0_2
 ; CHECK-NEXT:  .LBB0_3: # %for.end
+; CHECK-NEXT:    movzbl %al, %ecx


To address this can you add the OneUse checks back to the x86 version of isDesirableToCommuteWithShift?

RKSimon · 2023-10-15T13:11:50Z

llvm/lib/Target/X86/X86ISelLowering.cpp

+bool X86TargetLowering::isDesirableToCommuteWithShift(
+    const SDNode *N, CombineLevel Level) const {
+  if (N->getOperand(0)->hasOneUse())
+    return true;


This doesn't look like the hasOneUse is being applied to the same operand as the original?

topperc · 2023-10-15T22:33:29Z

Isn't this only profitable for RISC-V if the addi can be absorbed into a load/store address. Couldn't there still be other tests like the X86 case that would regress?

RKSimon · 2023-10-16T13:37:35Z

Are you intending to abandon this for #69105?

LiqinWeng · 2023-10-17T13:08:24Z

Are you intending to abandon this for #69105?

I found test of X86/pr65895.ll , llc -mtriple=riscv64 pr65895.ll , also regression(an extra register is used😂)；
this patch #69105， i will abandon；

LiqinWeng · 2023-10-17T13:15:37Z

Isn't this only profitable for RISC-V if the addi can be absorbed into a load/store address. Couldn't there still be other tests like the X86 case that would regress?

Test X86/pr65895.ll also regression（-mtriple=riscv64）
There is indeed no benefit in using x86 to test case riscv-shifted-extend.ll
So I don't know how to deal with it， should i remove the hasOneuse?

github-actions · 2023-10-20T05:49:21Z

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

…nsw x, c1)), c2) This patch remove the restriction for folding (shl (sext (add_nsw x, c1)), c2) -> (add (shl (sext x), c2), c1 << c2), and test case from dhrystone , see this link: https://godbolt.org/z/8zfa3rnad

LiqinWeng requested a review from RKSimon October 13, 2023 10:24

llvmbot added backend:RISC-V llvm:SelectionDAG SelectionDAGISel as well labels Oct 13, 2023

LiqinWeng force-pushed the shift-combine branch 2 times, most recently from 231dde8 to a939413 Compare October 13, 2023 15:12

llvmbot added the backend:X86 label Oct 13, 2023

goldsteinn reviewed Oct 13, 2023

View reviewed changes

LiqinWeng added a commit to LiqinWeng/llvm-project that referenced this pull request Oct 14, 2023

[Test] Pre-submit tests tofor llvm#68972

ed51ec2

LiqinWeng added a commit that referenced this pull request Oct 14, 2023

[Test] Pre-submit tests for #68972 (#69040)

64e7207

LiqinWeng force-pushed the shift-combine branch 2 times, most recently from c25cd56 to 3fff3b5 Compare October 14, 2023 05:20

LiqinWeng changed the title ~~[DAG] Remove restrictions and increase optimization opportunities~~ [DAG] Remove OneUse restriction on sext when folding (shl (sext (add_nsw x, c1)), c2) Oct 14, 2023

goldsteinn reviewed Oct 14, 2023

View reviewed changes

LiqinWeng force-pushed the shift-combine branch from 3fff3b5 to 04d12ce Compare October 15, 2023 04:18

RKSimon reviewed Oct 15, 2023

View reviewed changes

LiqinWeng mentioned this pull request Oct 16, 2023

[DAG] Remove OneUse restriction when folding (shl (add x, c1), c2) #69105

Closed

LiqinWeng force-pushed the shift-combine branch from 04d12ce to 9efabd4 Compare October 20, 2023 05:39

LiqinWeng force-pushed the shift-combine branch from 9efabd4 to f53dabb Compare October 20, 2023 06:59

madhur13490 mentioned this pull request Oct 20, 2023

Revert commit ba8565fbcb975e2d067ce3ae5a7dbaae4953edd3 madhur13490/llvm-project#3

Closed

LiqinWeng closed this Nov 19, 2023

LiqinWeng reopened this Jul 31, 2024

LiqinWeng added a commit to LiqinWeng/llvm-project that referenced this pull request Aug 3, 2024

[Test] Pre-submit tests for llvm#68972

9ea6962

LiqinWeng closed this Aug 9, 2024

LiqinWeng deleted the shift-combine branch November 1, 2024 06:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DAG] Remove OneUse restriction on sext when folding (shl (sext (add_nsw x, c1)), c2) #68972

[DAG] Remove OneUse restriction on sext when folding (shl (sext (add_nsw x, c1)), c2) #68972

LiqinWeng commented Oct 13, 2023 •

edited

Loading

llvmbot commented Oct 13, 2023 •

edited

Loading

goldsteinn commented Oct 13, 2023

goldsteinn Oct 13, 2023

LiqinWeng Oct 15, 2023

RKSimon Aug 5, 2024

goldsteinn Oct 14, 2023

LiqinWeng Oct 15, 2023

RKSimon Oct 15, 2023

goldsteinn Oct 15, 2023

topperc commented Oct 15, 2023

RKSimon commented Oct 16, 2023

LiqinWeng commented Oct 17, 2023

LiqinWeng commented Oct 17, 2023

github-actions bot commented Oct 20, 2023 •

edited

Loading

[DAG] Remove OneUse restriction on sext when folding (shl (sext (add_nsw x, c1)), c2) #68972

[DAG] Remove OneUse restriction on sext when folding (shl (sext (add_nsw x, c1)), c2) #68972

Conversation

LiqinWeng commented Oct 13, 2023 • edited Loading

llvmbot commented Oct 13, 2023 • edited Loading

goldsteinn commented Oct 13, 2023

goldsteinn Oct 13, 2023

Choose a reason for hiding this comment

LiqinWeng Oct 15, 2023

Choose a reason for hiding this comment

RKSimon Aug 5, 2024

Choose a reason for hiding this comment

goldsteinn Oct 14, 2023

Choose a reason for hiding this comment

LiqinWeng Oct 15, 2023

Choose a reason for hiding this comment

RKSimon Oct 15, 2023

Choose a reason for hiding this comment

goldsteinn Oct 15, 2023

Choose a reason for hiding this comment

topperc commented Oct 15, 2023

RKSimon commented Oct 16, 2023

LiqinWeng commented Oct 17, 2023

LiqinWeng commented Oct 17, 2023

github-actions bot commented Oct 20, 2023 • edited Loading

LiqinWeng commented Oct 13, 2023 •

edited

Loading

llvmbot commented Oct 13, 2023 •

edited

Loading

github-actions bot commented Oct 20, 2023 •

edited

Loading