[DAGCombiner][RISCV] Add target hook to decide hoisting LogicOp with extension #136677

tclin914 · 2025-04-22T10:10:31Z

This patch introduces a new target hook isDesirableToHoistLogicOpWithExt to allow target to decide hoisting LogicOp where both operands have the same extension op. By default it returns true.

On RISC-V, (or disjoint (sext/zext a), (sext/zext b)) can be combined as vwadd.vv/vwaddu.vv. So for such case, it returns false.

…extension. This patch introduces a new target hook `isDesirableToHoistLogicOpWithExt` to allow target to decide hoisting LogicOp where both operands have the same extension op. By default it returns true. On RISC-V, (or disjoint (sext/zext a), (sext/zext b)) can be combined as vwadd.vv/vwaddu.vv. So for such case, it returns false.

llvmbot · 2025-04-22T10:11:09Z

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

Author: Jim Lin (tclin914)

Changes

This patch introduces a new target hook isDesirableToHoistLogicOpWithExt to allow target to decide hoisting LogicOp where both operands have the same extension op. By default it returns true.

On RISC-V, (or disjoint (sext/zext a), (sext/zext b)) can be combined as vwadd.vv/vwaddu.vv. So for such case, it returns false.

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

5 Files Affected:

(modified) llvm/include/llvm/CodeGen/TargetLowering.h (+10)
(modified) llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp (+3)
(modified) llvm/lib/Target/RISCV/RISCVISelLowering.cpp (+8)
(modified) llvm/lib/Target/RISCV/RISCVISelLowering.h (+3)
(modified) llvm/test/CodeGen/RISCV/rvv/vwadd-sdnode.ll (+4-10)

diff --git a/llvm/include/llvm/CodeGen/TargetLowering.h b/llvm/include/llvm/CodeGen/TargetLowering.h
index 00c36266a069f..90a1d0e52d8c8 100644
--- a/llvm/include/llvm/CodeGen/TargetLowering.h
+++ b/llvm/include/llvm/CodeGen/TargetLowering.h
@@ -4462,6 +4462,16 @@ class TargetLowering : public TargetLoweringBase {
     return false;
   }
 
+  /// Return true if it is profitable to hoist a LogicOp where both operands
+  /// have the same extension op. This transformation may not be desirable if
+  /// it disrupts a particularly auspicious target-specific tree (e.g.
+  /// (or disjoint (zext A), (zext B)) -> vwaddu.wv on RISC-V). By default it
+  /// returns true.
+  virtual bool isDesirableToHoistLogicOpWithExt(const SDNode *LogicOp,
+                                                unsigned ExtOp) const {
+    return true;
+  }
+
   /// Return true if the target supports swifterror attribute. It optimizes
   /// loads and stores to reading and writing a specific register.
   virtual bool supportSwiftError() const {
diff --git a/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp b/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
index b175e35385ec6..6cbf2b6682929 100644
--- a/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
@@ -5981,6 +5981,9 @@ SDValue DAGCombiner::hoistLogicOpWithSameOpcodeHands(SDNode *N) {
          HandOpcode == ISD::ANY_EXTEND_VECTOR_INREG) &&
         LegalTypes && !TLI.isTypeDesirableForOp(LogicOpcode, XVT))
       return SDValue();
+    // If it is not desirable to hoist LogicOp with extension.
+    if (!TLI.isDesirableToHoistLogicOpWithExt(N, HandOpcode))
+      return SDValue();
     // logic_op (hand_op X), (hand_op Y) --> hand_op (logic_op X, Y)
     SDValue Logic = DAG.getNode(LogicOpcode, DL, XVT, X, Y);
     if (HandOpcode == ISD::SIGN_EXTEND_INREG)
diff --git a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
index 98fba9e86e88a..fe520c0298148 100644
--- a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
+++ b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
@@ -19893,6 +19893,14 @@ bool RISCVTargetLowering::isDesirableToCommuteWithShift(
   return true;
 }
 
+bool RISCVTargetLowering::isDesirableToHoistLogicOpWithExt(
+    const SDNode *LogicOp, unsigned ExtOp) const {
+  if (NodeExtensionHelper::isSupportedRoot(LogicOp, Subtarget) &&
+      (ExtOp == ISD::ZERO_EXTEND || ExtOp == ISD::SIGN_EXTEND))
+    return false;
+  return true;
+}
+
 bool RISCVTargetLowering::targetShrinkDemandedConstant(
     SDValue Op, const APInt &DemandedBits, const APInt &DemandedElts,
     TargetLoweringOpt &TLO) const {
diff --git a/llvm/lib/Target/RISCV/RISCVISelLowering.h b/llvm/lib/Target/RISCV/RISCVISelLowering.h
index baf1b2e4d8e6e..63d2fa95d5034 100644
--- a/llvm/lib/Target/RISCV/RISCVISelLowering.h
+++ b/llvm/lib/Target/RISCV/RISCVISelLowering.h
@@ -747,6 +747,9 @@ class RISCVTargetLowering : public TargetLowering {
   bool isDesirableToCommuteWithShift(const SDNode *N,
                                      CombineLevel Level) const override;
 
+  bool isDesirableToHoistLogicOpWithExt(const SDNode *LogicOp,
+                                        unsigned ExtOp) const override;
+
   /// If a physical register, this returns the register that receives the
   /// exception address on entry to an EH pad.
   Register
diff --git a/llvm/test/CodeGen/RISCV/rvv/vwadd-sdnode.ll b/llvm/test/CodeGen/RISCV/rvv/vwadd-sdnode.ll
index 3f5d42f89337b..149950484c477 100644
--- a/llvm/test/CodeGen/RISCV/rvv/vwadd-sdnode.ll
+++ b/llvm/test/CodeGen/RISCV/rvv/vwadd-sdnode.ll
@@ -1417,15 +1417,12 @@ define <vscale x 2 x i32> @vwaddu_vv_disjoint_or_add(<vscale x 2 x i8> %x.i8, <v
   ret <vscale x 2 x i32> %add
 }
 
-; TODO: We could select vwaddu.vv, but when both arms of the or are the same
-; DAGCombiner::hoistLogicOpWithSameOpcodeHands moves the zext above the or.
 define <vscale x 2 x i32> @vwaddu_vv_disjoint_or(<vscale x 2 x i16> %x.i16, <vscale x 2 x i16> %y.i16) {
 ; CHECK-LABEL: vwaddu_vv_disjoint_or:
 ; CHECK:       # %bb.0:
 ; CHECK-NEXT:    vsetvli a0, zero, e16, mf2, ta, ma
-; CHECK-NEXT:    vor.vv v9, v8, v9
-; CHECK-NEXT:    vsetvli zero, zero, e32, m1, ta, ma
-; CHECK-NEXT:    vzext.vf2 v8, v9
+; CHECK-NEXT:    vwaddu.vv v10, v8, v9
+; CHECK-NEXT:    vmv1r.v v8, v10
 ; CHECK-NEXT:    ret
   %x.i32 = zext <vscale x 2 x i16> %x.i16 to <vscale x 2 x i32>
   %y.i32 = zext <vscale x 2 x i16> %y.i16 to <vscale x 2 x i32>
@@ -1433,15 +1430,12 @@ define <vscale x 2 x i32> @vwaddu_vv_disjoint_or(<vscale x 2 x i16> %x.i16, <vsc
   ret <vscale x 2 x i32> %or
 }
 
-; TODO: We could select vwadd.vv, but when both arms of the or are the same
-; DAGCombiner::hoistLogicOpWithSameOpcodeHands moves the zext above the or.
 define <vscale x 2 x i32> @vwadd_vv_disjoint_or(<vscale x 2 x i16> %x.i16, <vscale x 2 x i16> %y.i16) {
 ; CHECK-LABEL: vwadd_vv_disjoint_or:
 ; CHECK:       # %bb.0:
 ; CHECK-NEXT:    vsetvli a0, zero, e16, mf2, ta, ma
-; CHECK-NEXT:    vor.vv v9, v8, v9
-; CHECK-NEXT:    vsetvli zero, zero, e32, m1, ta, ma
-; CHECK-NEXT:    vsext.vf2 v8, v9
+; CHECK-NEXT:    vwadd.vv v10, v8, v9
+; CHECK-NEXT:    vmv1r.v v8, v10
 ; CHECK-NEXT:    ret
   %x.i32 = sext <vscale x 2 x i16> %x.i16 to <vscale x 2 x i32>
   %y.i32 = sext <vscale x 2 x i16> %y.i16 to <vscale x 2 x i32>

llvmbot · 2025-04-22T10:11:10Z

@llvm/pr-subscribers-llvm-selectiondag

Author: Jim Lin (tclin914)

Changes

This patch introduces a new target hook isDesirableToHoistLogicOpWithExt to allow target to decide hoisting LogicOp where both operands have the same extension op. By default it returns true.

On RISC-V, (or disjoint (sext/zext a), (sext/zext b)) can be combined as vwadd.vv/vwaddu.vv. So for such case, it returns false.

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

5 Files Affected:

(modified) llvm/include/llvm/CodeGen/TargetLowering.h (+10)
(modified) llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp (+3)
(modified) llvm/lib/Target/RISCV/RISCVISelLowering.cpp (+8)
(modified) llvm/lib/Target/RISCV/RISCVISelLowering.h (+3)
(modified) llvm/test/CodeGen/RISCV/rvv/vwadd-sdnode.ll (+4-10)

diff --git a/llvm/include/llvm/CodeGen/TargetLowering.h b/llvm/include/llvm/CodeGen/TargetLowering.h
index 00c36266a069f..90a1d0e52d8c8 100644
--- a/llvm/include/llvm/CodeGen/TargetLowering.h
+++ b/llvm/include/llvm/CodeGen/TargetLowering.h
@@ -4462,6 +4462,16 @@ class TargetLowering : public TargetLoweringBase {
     return false;
   }
 
+  /// Return true if it is profitable to hoist a LogicOp where both operands
+  /// have the same extension op. This transformation may not be desirable if
+  /// it disrupts a particularly auspicious target-specific tree (e.g.
+  /// (or disjoint (zext A), (zext B)) -> vwaddu.wv on RISC-V). By default it
+  /// returns true.
+  virtual bool isDesirableToHoistLogicOpWithExt(const SDNode *LogicOp,
+                                                unsigned ExtOp) const {
+    return true;
+  }
+
   /// Return true if the target supports swifterror attribute. It optimizes
   /// loads and stores to reading and writing a specific register.
   virtual bool supportSwiftError() const {
diff --git a/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp b/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
index b175e35385ec6..6cbf2b6682929 100644
--- a/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
@@ -5981,6 +5981,9 @@ SDValue DAGCombiner::hoistLogicOpWithSameOpcodeHands(SDNode *N) {
          HandOpcode == ISD::ANY_EXTEND_VECTOR_INREG) &&
         LegalTypes && !TLI.isTypeDesirableForOp(LogicOpcode, XVT))
       return SDValue();
+    // If it is not desirable to hoist LogicOp with extension.
+    if (!TLI.isDesirableToHoistLogicOpWithExt(N, HandOpcode))
+      return SDValue();
     // logic_op (hand_op X), (hand_op Y) --> hand_op (logic_op X, Y)
     SDValue Logic = DAG.getNode(LogicOpcode, DL, XVT, X, Y);
     if (HandOpcode == ISD::SIGN_EXTEND_INREG)
diff --git a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
index 98fba9e86e88a..fe520c0298148 100644
--- a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
+++ b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
@@ -19893,6 +19893,14 @@ bool RISCVTargetLowering::isDesirableToCommuteWithShift(
   return true;
 }
 
+bool RISCVTargetLowering::isDesirableToHoistLogicOpWithExt(
+    const SDNode *LogicOp, unsigned ExtOp) const {
+  if (NodeExtensionHelper::isSupportedRoot(LogicOp, Subtarget) &&
+      (ExtOp == ISD::ZERO_EXTEND || ExtOp == ISD::SIGN_EXTEND))
+    return false;
+  return true;
+}
+
 bool RISCVTargetLowering::targetShrinkDemandedConstant(
     SDValue Op, const APInt &DemandedBits, const APInt &DemandedElts,
     TargetLoweringOpt &TLO) const {
diff --git a/llvm/lib/Target/RISCV/RISCVISelLowering.h b/llvm/lib/Target/RISCV/RISCVISelLowering.h
index baf1b2e4d8e6e..63d2fa95d5034 100644
--- a/llvm/lib/Target/RISCV/RISCVISelLowering.h
+++ b/llvm/lib/Target/RISCV/RISCVISelLowering.h
@@ -747,6 +747,9 @@ class RISCVTargetLowering : public TargetLowering {
   bool isDesirableToCommuteWithShift(const SDNode *N,
                                      CombineLevel Level) const override;
 
+  bool isDesirableToHoistLogicOpWithExt(const SDNode *LogicOp,
+                                        unsigned ExtOp) const override;
+
   /// If a physical register, this returns the register that receives the
   /// exception address on entry to an EH pad.
   Register
diff --git a/llvm/test/CodeGen/RISCV/rvv/vwadd-sdnode.ll b/llvm/test/CodeGen/RISCV/rvv/vwadd-sdnode.ll
index 3f5d42f89337b..149950484c477 100644
--- a/llvm/test/CodeGen/RISCV/rvv/vwadd-sdnode.ll
+++ b/llvm/test/CodeGen/RISCV/rvv/vwadd-sdnode.ll
@@ -1417,15 +1417,12 @@ define <vscale x 2 x i32> @vwaddu_vv_disjoint_or_add(<vscale x 2 x i8> %x.i8, <v
   ret <vscale x 2 x i32> %add
 }
 
-; TODO: We could select vwaddu.vv, but when both arms of the or are the same
-; DAGCombiner::hoistLogicOpWithSameOpcodeHands moves the zext above the or.
 define <vscale x 2 x i32> @vwaddu_vv_disjoint_or(<vscale x 2 x i16> %x.i16, <vscale x 2 x i16> %y.i16) {
 ; CHECK-LABEL: vwaddu_vv_disjoint_or:
 ; CHECK:       # %bb.0:
 ; CHECK-NEXT:    vsetvli a0, zero, e16, mf2, ta, ma
-; CHECK-NEXT:    vor.vv v9, v8, v9
-; CHECK-NEXT:    vsetvli zero, zero, e32, m1, ta, ma
-; CHECK-NEXT:    vzext.vf2 v8, v9
+; CHECK-NEXT:    vwaddu.vv v10, v8, v9
+; CHECK-NEXT:    vmv1r.v v8, v10
 ; CHECK-NEXT:    ret
   %x.i32 = zext <vscale x 2 x i16> %x.i16 to <vscale x 2 x i32>
   %y.i32 = zext <vscale x 2 x i16> %y.i16 to <vscale x 2 x i32>
@@ -1433,15 +1430,12 @@ define <vscale x 2 x i32> @vwaddu_vv_disjoint_or(<vscale x 2 x i16> %x.i16, <vsc
   ret <vscale x 2 x i32> %or
 }
 
-; TODO: We could select vwadd.vv, but when both arms of the or are the same
-; DAGCombiner::hoistLogicOpWithSameOpcodeHands moves the zext above the or.
 define <vscale x 2 x i32> @vwadd_vv_disjoint_or(<vscale x 2 x i16> %x.i16, <vscale x 2 x i16> %y.i16) {
 ; CHECK-LABEL: vwadd_vv_disjoint_or:
 ; CHECK:       # %bb.0:
 ; CHECK-NEXT:    vsetvli a0, zero, e16, mf2, ta, ma
-; CHECK-NEXT:    vor.vv v9, v8, v9
-; CHECK-NEXT:    vsetvli zero, zero, e32, m1, ta, ma
-; CHECK-NEXT:    vsext.vf2 v8, v9
+; CHECK-NEXT:    vwadd.vv v10, v8, v9
+; CHECK-NEXT:    vmv1r.v v8, v10
 ; CHECK-NEXT:    ret
   %x.i32 = sext <vscale x 2 x i16> %x.i16 to <vscale x 2 x i32>
   %y.i32 = sext <vscale x 2 x i16> %y.i16 to <vscale x 2 x i32>

wangpc-pp

LGTM.

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

llvm/include/llvm/CodeGen/TargetLowering.h

lukel97

LGTM. I wonder if we should just add a pattern for disjoint ors rather than adding yet another target hook for DAGCombiner. But I think we can explore that in a follow up

DAGCombiner::hoistLogicOpWithSameOpcodeHands will hoist (or disjoint (ext a), (ext b)) -> (ext (or disjoint a, b)) So this adds a pattern to match vwadd[u].vv in this case. We have to teach the combine to preserve the disjoint flag, and add a generic PatFrag for a disjoint or. This is meant to be a follow up to llvm#136677 which would allow us to remove the target hook added there.

mshockwave

LGTM w/ one nit comment

mshockwave · 2025-04-22T16:12:36Z

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

+  if ((ExtOp == ISD::ZERO_EXTEND || ExtOp == ISD::SIGN_EXTEND) &&
+      NodeExtensionHelper::isSupportedRoot(LogicOp, Subtarget))
+    return false;
+  return true;


maybe combine this into a single return?

topperc · 2025-04-22T16:32:49Z

Doesn't InstCombine do the same transform as isDesirableToHoistLogicOpWithExt? https://godbolt.org/z/rM3dWh83e

Do we have a transform to turn it back into (or disjoint (sext/zext a), (sext/zext b))? If not then this patch isn't all that useful by itself.

arsenm · 2025-04-22T16:45:18Z

llvm/include/llvm/CodeGen/TargetLowering.h

+  virtual bool isDesirableToHoistLogicOpWithExt(const SDNode *LogicOp,
+                                                unsigned ExtOp) const {
+    return true;
+  }


Can't you just select on the hoisted form of the pattern

See #136716

tclin914 · 2025-04-23T05:56:48Z

Doesn't InstCombine do the same transform as isDesirableToHoistLogicOpWithExt? https://godbolt.org/z/rM3dWh83e

Do we have a transform to turn it back into (or disjoint (sext/zext a), (sext/zext b))? If not then this patch isn't all that useful by itself.

Keep disjoint #136815 if it is transformed during InstCombine.

tclin914 requested review from lukel97, topperc and wangpc-pp April 22, 2025 10:10

llvmbot added backend:RISC-V llvm:SelectionDAG SelectionDAGISel as well labels Apr 22, 2025

wangpc-pp approved these changes Apr 22, 2025

View reviewed changes

llvm/lib/Target/RISCV/RISCVISelLowering.cpp Outdated Show resolved Hide resolved

lukel97 reviewed Apr 22, 2025

View reviewed changes

llvm/lib/Target/RISCV/RISCVISelLowering.cpp Outdated Show resolved Hide resolved

tclin914 added 3 commits April 22, 2025 21:40

Move the condition forward

3b1f464

Drop the comment

a427e70

Typo: vwaddu.wv -> vwaddu.vv

482abae

lukel97 reviewed Apr 22, 2025

View reviewed changes

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp Show resolved Hide resolved

llvm/include/llvm/CodeGen/TargetLowering.h Outdated Show resolved Hide resolved

lukel97 approved these changes Apr 22, 2025

View reviewed changes

lukel97 mentioned this pull request Apr 22, 2025

[RISCV] Add disjoint or patterns for vwadd[u].vv #136716

Merged

mshockwave approved these changes Apr 22, 2025

View reviewed changes

arsenm reviewed Apr 22, 2025

View reviewed changes

tclin914 closed this May 15, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[DAGCombiner][RISCV] Add target hook to decide hoisting LogicOp with extension #136677

[DAGCombiner][RISCV] Add target hook to decide hoisting LogicOp with extension #136677

Uh oh!

tclin914 commented Apr 22, 2025

Uh oh!

llvmbot commented Apr 22, 2025

Uh oh!

llvmbot commented Apr 22, 2025

Uh oh!

wangpc-pp left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lukel97 left a comment

Uh oh!

mshockwave left a comment

Uh oh!

mshockwave Apr 22, 2025

Uh oh!

topperc commented Apr 22, 2025 •

edited

Loading

Uh oh!

arsenm Apr 22, 2025

Uh oh!

lukel97 Apr 22, 2025

Uh oh!

tclin914 commented Apr 23, 2025

Uh oh!

Uh oh!

[DAGCombiner][RISCV] Add target hook to decide hoisting LogicOp with extension #136677

[DAGCombiner][RISCV] Add target hook to decide hoisting LogicOp with extension #136677

Uh oh!

Conversation

tclin914 commented Apr 22, 2025

Uh oh!

llvmbot commented Apr 22, 2025

Uh oh!

llvmbot commented Apr 22, 2025

Uh oh!

wangpc-pp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lukel97 left a comment

Choose a reason for hiding this comment

Uh oh!

mshockwave left a comment

Choose a reason for hiding this comment

Uh oh!

mshockwave Apr 22, 2025

Choose a reason for hiding this comment

Uh oh!

topperc commented Apr 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arsenm Apr 22, 2025

Choose a reason for hiding this comment

Uh oh!

lukel97 Apr 22, 2025

Choose a reason for hiding this comment

Uh oh!

tclin914 commented Apr 23, 2025

Uh oh!

Uh oh!

topperc commented Apr 22, 2025 •

edited

Loading