[SelectionDAG][RISCV] Fix break of vnsrl pattern in issue #94265 #95563

Fros1er · 2024-06-14T16:37:34Z

Added a overload of isTypeDesirableForOp to take NewVT + OldVT, fixing the break of vnsrl described in issue #94265.

…issue#94265

github-actions · 2024-06-14T16:37:50Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

llvmbot · 2024-06-14T16:38:20Z

@llvm/pr-subscribers-llvm-selectiondag

Author: Froster (Fros1er)

Changes

Added a overload of isTypeDesirableForOp to take NewVT + OldVT, fixing the break of vnsrl described in issue #94265.

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

5 Files Affected:

(modified) llvm/include/llvm/CodeGen/TargetLowering.h (+14)
(modified) llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp (+3-1)
(modified) llvm/lib/Target/RISCV/RISCVISelLowering.cpp (+8)
(modified) llvm/lib/Target/RISCV/RISCVISelLowering.h (+2)
(added) llvm/test/CodeGen/RISCV/pr94265.ll (+31)

diff --git a/llvm/include/llvm/CodeGen/TargetLowering.h b/llvm/include/llvm/CodeGen/TargetLowering.h
index 3074ece787a08..f0e20e4372b8d 100644
--- a/llvm/include/llvm/CodeGen/TargetLowering.h
+++ b/llvm/include/llvm/CodeGen/TargetLowering.h
@@ -4339,6 +4339,20 @@ class TargetLowering : public TargetLoweringBase {
     return isTypeLegal(VT);
   }
 
+  /// Same as isTypeDesirableForOp(unsigned Opc, EVT VT), but also check if
+  /// the target is 'desirable' to truncate or extend OldVT to NewVT only using
+  /// the given node type, without the need of explicit trunc or ext. e.g. On
+  /// RISC-V Vector extension, vnsrl.wi can directly convert <n x i32> to <n x
+  /// i16> when shifting, with no extra trunc operations needed.
+  virtual bool isTypeDesirableForOp(unsigned Opc, EVT NewVT, EVT OldVT) const {
+    // Fallback to isTypeDesirableForOp(unsigned Opc, EVT VT).
+    if (NewVT == OldVT) {
+      return isTypeDesirableForOp(Opc, NewVT);
+    }
+    // Most of instructions are not desirable, so return false by default.
+    return false;
+  }
+
   /// Return true if it is profitable for dag combiner to transform a floating
   /// point op of specified opcode to a equivalent op of an integer
   /// type. e.g. f32 load -> i32 load can be profitable on ARM.
diff --git a/llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp b/llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
index 623d2e0a047ef..373aeac5e7317 100644
--- a/llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
@@ -2597,7 +2597,9 @@ bool TargetLowering::SimplifyDemandedBits(
         HighBits.lshrInPlace(ShVal);
         HighBits = HighBits.trunc(BitWidth);
 
-        if (!(HighBits & DemandedBits)) {
+        if (!isTypeDesirableForOp(ISD::SRL, Op.getValueType(),
+                                  Src.getValueType()) &&
+            !(HighBits & DemandedBits)) {
           // None of the shifted in bits are needed.  Add a truncate of the
           // shift input, then shift it.
           SDValue NewShAmt =
diff --git a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
index b1b27f03252e0..694e0b0dff1a3 100644
--- a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
+++ b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
@@ -17462,6 +17462,14 @@ bool RISCVTargetLowering::isDesirableToCommuteWithShift(
   return true;
 }
 
+bool RISCVTargetLowering::isTypeDesirableForOp(unsigned Opc, EVT NewVT,
+                                               EVT OldVT) const {
+  if (Subtarget.hasStdExtV() && NewVT.isVector() && OldVT.isVector()) {
+    return true;
+  }
+  return TargetLowering::isTypeDesirableForOp(Opc, NewVT, OldVT);
+}
+
 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 3b8eb3c88901a..353836783ccfb 100644
--- a/llvm/lib/Target/RISCV/RISCVISelLowering.h
+++ b/llvm/lib/Target/RISCV/RISCVISelLowering.h
@@ -708,6 +708,8 @@ class RISCVTargetLowering : public TargetLowering {
   bool isDesirableToCommuteWithShift(const SDNode *N,
                                      CombineLevel Level) const override;
 
+  bool isTypeDesirableForOp(unsigned Opc, EVT NewVT, EVT OldVT) 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/pr94265.ll b/llvm/test/CodeGen/RISCV/pr94265.ll
new file mode 100644
index 0000000000000..cb41e22381d19
--- /dev/null
+++ b/llvm/test/CodeGen/RISCV/pr94265.ll
@@ -0,0 +1,31 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc < %s -mtriple=riscv32-- -mattr=+v | FileCheck -check-prefix=RV32I %s
+; RUN: llc < %s -mtriple=riscv64-- -mattr=+v | FileCheck -check-prefix=RV64I %s
+
+define <8 x i16> @PR94265(<8 x i32> %a0) #0 {
+; RV32I-LABEL: PR94265:
+; RV32I:       # %bb.0:
+; RV32I-NEXT:    vsetivli zero, 8, e32, m2, ta, ma
+; RV32I-NEXT:    vsra.vi v10, v8, 31
+; RV32I-NEXT:    vsrl.vi v10, v10, 26
+; RV32I-NEXT:    vadd.vv v8, v8, v10
+; RV32I-NEXT:    vsetvli zero, zero, e16, m1, ta, ma
+; RV32I-NEXT:    vnsrl.wi v10, v8, 6
+; RV32I-NEXT:    vsll.vi v8, v10, 10
+; RV32I-NEXT:    ret
+;
+; RV64I-LABEL: PR94265:
+; RV64I:       # %bb.0:
+; RV64I-NEXT:    vsetivli zero, 8, e32, m2, ta, ma
+; RV64I-NEXT:    vsra.vi v10, v8, 31
+; RV64I-NEXT:    vsrl.vi v10, v10, 26
+; RV64I-NEXT:    vadd.vv v8, v8, v10
+; RV64I-NEXT:    vsetvli zero, zero, e16, m1, ta, ma
+; RV64I-NEXT:    vnsrl.wi v10, v8, 6
+; RV64I-NEXT:    vsll.vi v8, v10, 10
+; RV64I-NEXT:    ret
+  %t1 = sdiv <8 x i32> %a0, <i32 64, i32 64, i32 64, i32 64, i32 64, i32 64, i32 64, i32 64>
+  %t2 = trunc <8 x i32> %t1 to <8 x i16>
+  %t3 = shl <8 x i16> %t2, <i16 10, i16 10, i16 10, i16 10, i16 10, i16 10, i16 10, i16 10>
+  ret <8 x i16> %t3
+}

llvmbot · 2024-06-14T16:38:21Z

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

Author: Froster (Fros1er)

Changes

Added a overload of isTypeDesirableForOp to take NewVT + OldVT, fixing the break of vnsrl described in issue #94265.

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

5 Files Affected:

(modified) llvm/include/llvm/CodeGen/TargetLowering.h (+14)
(modified) llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp (+3-1)
(modified) llvm/lib/Target/RISCV/RISCVISelLowering.cpp (+8)
(modified) llvm/lib/Target/RISCV/RISCVISelLowering.h (+2)
(added) llvm/test/CodeGen/RISCV/pr94265.ll (+31)

diff --git a/llvm/include/llvm/CodeGen/TargetLowering.h b/llvm/include/llvm/CodeGen/TargetLowering.h
index 3074ece787a08..f0e20e4372b8d 100644
--- a/llvm/include/llvm/CodeGen/TargetLowering.h
+++ b/llvm/include/llvm/CodeGen/TargetLowering.h
@@ -4339,6 +4339,20 @@ class TargetLowering : public TargetLoweringBase {
     return isTypeLegal(VT);
   }
 
+  /// Same as isTypeDesirableForOp(unsigned Opc, EVT VT), but also check if
+  /// the target is 'desirable' to truncate or extend OldVT to NewVT only using
+  /// the given node type, without the need of explicit trunc or ext. e.g. On
+  /// RISC-V Vector extension, vnsrl.wi can directly convert <n x i32> to <n x
+  /// i16> when shifting, with no extra trunc operations needed.
+  virtual bool isTypeDesirableForOp(unsigned Opc, EVT NewVT, EVT OldVT) const {
+    // Fallback to isTypeDesirableForOp(unsigned Opc, EVT VT).
+    if (NewVT == OldVT) {
+      return isTypeDesirableForOp(Opc, NewVT);
+    }
+    // Most of instructions are not desirable, so return false by default.
+    return false;
+  }
+
   /// Return true if it is profitable for dag combiner to transform a floating
   /// point op of specified opcode to a equivalent op of an integer
   /// type. e.g. f32 load -> i32 load can be profitable on ARM.
diff --git a/llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp b/llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
index 623d2e0a047ef..373aeac5e7317 100644
--- a/llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
@@ -2597,7 +2597,9 @@ bool TargetLowering::SimplifyDemandedBits(
         HighBits.lshrInPlace(ShVal);
         HighBits = HighBits.trunc(BitWidth);
 
-        if (!(HighBits & DemandedBits)) {
+        if (!isTypeDesirableForOp(ISD::SRL, Op.getValueType(),
+                                  Src.getValueType()) &&
+            !(HighBits & DemandedBits)) {
           // None of the shifted in bits are needed.  Add a truncate of the
           // shift input, then shift it.
           SDValue NewShAmt =
diff --git a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
index b1b27f03252e0..694e0b0dff1a3 100644
--- a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
+++ b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
@@ -17462,6 +17462,14 @@ bool RISCVTargetLowering::isDesirableToCommuteWithShift(
   return true;
 }
 
+bool RISCVTargetLowering::isTypeDesirableForOp(unsigned Opc, EVT NewVT,
+                                               EVT OldVT) const {
+  if (Subtarget.hasStdExtV() && NewVT.isVector() && OldVT.isVector()) {
+    return true;
+  }
+  return TargetLowering::isTypeDesirableForOp(Opc, NewVT, OldVT);
+}
+
 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 3b8eb3c88901a..353836783ccfb 100644
--- a/llvm/lib/Target/RISCV/RISCVISelLowering.h
+++ b/llvm/lib/Target/RISCV/RISCVISelLowering.h
@@ -708,6 +708,8 @@ class RISCVTargetLowering : public TargetLowering {
   bool isDesirableToCommuteWithShift(const SDNode *N,
                                      CombineLevel Level) const override;
 
+  bool isTypeDesirableForOp(unsigned Opc, EVT NewVT, EVT OldVT) 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/pr94265.ll b/llvm/test/CodeGen/RISCV/pr94265.ll
new file mode 100644
index 0000000000000..cb41e22381d19
--- /dev/null
+++ b/llvm/test/CodeGen/RISCV/pr94265.ll
@@ -0,0 +1,31 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc < %s -mtriple=riscv32-- -mattr=+v | FileCheck -check-prefix=RV32I %s
+; RUN: llc < %s -mtriple=riscv64-- -mattr=+v | FileCheck -check-prefix=RV64I %s
+
+define <8 x i16> @PR94265(<8 x i32> %a0) #0 {
+; RV32I-LABEL: PR94265:
+; RV32I:       # %bb.0:
+; RV32I-NEXT:    vsetivli zero, 8, e32, m2, ta, ma
+; RV32I-NEXT:    vsra.vi v10, v8, 31
+; RV32I-NEXT:    vsrl.vi v10, v10, 26
+; RV32I-NEXT:    vadd.vv v8, v8, v10
+; RV32I-NEXT:    vsetvli zero, zero, e16, m1, ta, ma
+; RV32I-NEXT:    vnsrl.wi v10, v8, 6
+; RV32I-NEXT:    vsll.vi v8, v10, 10
+; RV32I-NEXT:    ret
+;
+; RV64I-LABEL: PR94265:
+; RV64I:       # %bb.0:
+; RV64I-NEXT:    vsetivli zero, 8, e32, m2, ta, ma
+; RV64I-NEXT:    vsra.vi v10, v8, 31
+; RV64I-NEXT:    vsrl.vi v10, v10, 26
+; RV64I-NEXT:    vadd.vv v8, v8, v10
+; RV64I-NEXT:    vsetvli zero, zero, e16, m1, ta, ma
+; RV64I-NEXT:    vnsrl.wi v10, v8, 6
+; RV64I-NEXT:    vsll.vi v8, v10, 10
+; RV64I-NEXT:    ret
+  %t1 = sdiv <8 x i32> %a0, <i32 64, i32 64, i32 64, i32 64, i32 64, i32 64, i32 64, i32 64>
+  %t2 = trunc <8 x i32> %t1 to <8 x i16>
+  %t3 = shl <8 x i16> %t2, <i16 10, i16 10, i16 10, i16 10, i16 10, i16 10, i16 10, i16 10>
+  ret <8 x i16> %t3
+}

llvm/include/llvm/CodeGen/TargetLowering.h

RKSimon

This breaks a number of tests:

Failed Tests (8):
  LLVM :: CodeGen/AArch64/bitfield-extract.ll
  LLVM :: CodeGen/AArch64/extbinopload.ll
  LLVM :: CodeGen/AArch64/trunc-to-tbl.ll
  LLVM :: CodeGen/AArch64/zext-to-tbl.ll
  LLVM :: CodeGen/AMDGPU/amdgpu-codegenprepare-idiv.ll
  LLVM :: CodeGen/Hexagon/two-crash.ll
  LLVM :: CodeGen/Thumb2/shift_parts.ll
  LLVM :: CodeGen/X86/pr44915.ll

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

RKSimon

LGTM with one minor query

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

PR Link: llvm/llvm-project#95563

github-actions · 2024-07-14T11:09:54Z

@Fros1er Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested
by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as
the builds can include changes from many authors. It is not uncommon for your
change to be included in a build that fails due to someone else's changes, or
infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself.
This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

…lvm#95563) Added a RISCV overload of `isTruncateFree` to fix the break of vnsrl described in issue llvm#94265. Fixes llvm#94265

Fros1er added 2 commits June 14, 2024 23:49

[SelectionDAG][RISCV] Add pre-commit tests.

a2018ef

[SelectionDAG][RISCV] Add isTypeDesirableForOp with NewVT+OldVT, fix …

2c04c23

…issue#94265

llvmbot added backend:RISC-V llvm:SelectionDAG SelectionDAGISel as well labels Jun 14, 2024

arsenm reviewed Jun 14, 2024

View reviewed changes

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

dtcxzyw requested review from RKSimon, topperc and dtcxzyw June 14, 2024 16:46

rename new func to isTypeDesirableForOpwithCast

f6ab73c

dtcxzyw requested review from lukel97 and wangpc-pp June 28, 2024 14:46

RKSimon reviewed Jun 28, 2024

View reviewed changes

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

Fros1er added 2 commits June 30, 2024 03:30

remove new func, use overrided isTruncateFree instead

6286d17

format

261506d

Fros1er requested a review from RKSimon June 30, 2024 11:55

RKSimon requested changes Jul 1, 2024

View reviewed changes

lukel97 reviewed Jul 1, 2024

View reviewed changes

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

fix failed tests, check size of VT in isTruncateFree

4ba7e48

Fros1er requested a review from RKSimon July 12, 2024 06:52

RKSimon approved these changes Jul 12, 2024

View reviewed changes

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

fallback when srcbits != destbits

a5a8af6

dtcxzyw added a commit to dtcxzyw/llvm-codegen-benchmark that referenced this pull request Jul 12, 2024

pre-commit: test PR95563

a6d7436

PR Link: llvm/llvm-project#95563

dtcxzyw mentioned this pull request Jul 12, 2024

pre-commit: test PR95563 dtcxzyw/llvm-codegen-benchmark#77

Closed

Fros1er requested a review from RKSimon July 13, 2024 07:21

RKSimon approved these changes Jul 14, 2024

View reviewed changes

RKSimon merged commit c8dc21d into llvm:main Jul 14, 2024
7 checks passed

dtcxzyw mentioned this pull request Jul 14, 2024

Update diff June 1st 2024, 5:00:55 pm dtcxzyw/llvm-codegen-benchmark#62

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SelectionDAG][RISCV] Fix break of vnsrl pattern in issue #94265 #95563

[SelectionDAG][RISCV] Fix break of vnsrl pattern in issue #94265 #95563

Fros1er commented Jun 14, 2024 •

edited by RKSimon

Loading

github-actions bot commented Jun 14, 2024

llvmbot commented Jun 14, 2024

llvmbot commented Jun 14, 2024

RKSimon left a comment

RKSimon left a comment

github-actions bot commented Jul 14, 2024

[SelectionDAG][RISCV] Fix break of vnsrl pattern in issue #94265 #95563

[SelectionDAG][RISCV] Fix break of vnsrl pattern in issue #94265 #95563

Conversation

Fros1er commented Jun 14, 2024 • edited by RKSimon Loading

github-actions bot commented Jun 14, 2024

llvmbot commented Jun 14, 2024

llvmbot commented Jun 14, 2024

RKSimon left a comment

Choose a reason for hiding this comment

RKSimon left a comment

Choose a reason for hiding this comment

github-actions bot commented Jul 14, 2024

Fros1er commented Jun 14, 2024 •

edited by RKSimon

Loading