Skip to content

Conversation

@llvmbot
Copy link
Member

@llvmbot llvmbot commented Dec 27, 2024

Backport 0019565

Requested by: @topperc

…x. (llvm#116159)

The fix in ReplaceNodeResults is the only one really required for the
known crash.

I couldn't hit the case in LowerOperation because that requires (f64
(bitcast i64)), but the result type is softened before the input so we
don't get a chance to legalize the input.

The change to the setOperationAction call was an observation that a
i64<->vector cast should not be custom legalized on RV32. The custom
code already calls isTypeLegal on the scalar type.

(cherry picked from commit 0019565)
@llvmbot llvmbot added this to the LLVM 19.X Release milestone Dec 27, 2024
@llvmbot
Copy link
Member Author

llvmbot commented Dec 27, 2024

@lenary What do you think about merging this PR to the release branch?

@llvmbot
Copy link
Member Author

llvmbot commented Dec 27, 2024

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

Author: None (llvmbot)

Changes

Backport 0019565

Requested by: @topperc


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

2 Files Affected:

  • (modified) llvm/lib/Target/RISCV/RISCVISelLowering.cpp (+7-4)
  • (added) llvm/test/CodeGen/RISCV/rvv/rv32-zve-bitcast-crash.ll (+24)
diff --git a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
index 823fb428472ef3..badbb425997447 100644
--- a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
+++ b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
@@ -1396,8 +1396,9 @@ RISCVTargetLowering::RISCVTargetLowering(const TargetMachine &TM,
       }
 
       // Custom-legalize bitcasts from fixed-length vectors to scalar types.
-      setOperationAction(ISD::BITCAST, {MVT::i8, MVT::i16, MVT::i32, MVT::i64},
-                         Custom);
+      setOperationAction(ISD::BITCAST, {MVT::i8, MVT::i16, MVT::i32}, Custom);
+      if (Subtarget.is64Bit())
+        setOperationAction(ISD::BITCAST, MVT::i64, Custom);
       if (Subtarget.hasStdExtZfhminOrZhinxmin())
         setOperationAction(ISD::BITCAST, MVT::f16, Custom);
       if (Subtarget.hasStdExtFOrZfinx())
@@ -6317,7 +6318,8 @@ SDValue RISCVTargetLowering::LowerOperation(SDValue Op,
           DAG.getNode(RISCVISD::FMV_W_X_RV64, DL, MVT::f32, NewOp0);
       return FPConv;
     }
-    if (VT == MVT::f64 && Op0VT == MVT::i64 && XLenVT == MVT::i32) {
+    if (VT == MVT::f64 && Op0VT == MVT::i64 && !Subtarget.is64Bit() &&
+        Subtarget.hasStdExtDOrZdinx()) {
       SDValue Lo, Hi;
       std::tie(Lo, Hi) = DAG.SplitScalar(Op0, DL, MVT::i32, MVT::i32);
       SDValue RetReg =
@@ -12616,7 +12618,8 @@ void RISCVTargetLowering::ReplaceNodeResults(SDNode *N,
       SDValue FPConv =
           DAG.getNode(RISCVISD::FMV_X_ANYEXTW_RV64, DL, MVT::i64, Op0);
       Results.push_back(DAG.getNode(ISD::TRUNCATE, DL, MVT::i32, FPConv));
-    } else if (VT == MVT::i64 && Op0VT == MVT::f64 && XLenVT == MVT::i32) {
+    } else if (VT == MVT::i64 && Op0VT == MVT::f64 && !Subtarget.is64Bit() &&
+               Subtarget.hasStdExtDOrZdinx()) {
       SDValue NewReg = DAG.getNode(RISCVISD::SplitF64, DL,
                                    DAG.getVTList(MVT::i32, MVT::i32), Op0);
       SDValue RetReg = DAG.getNode(ISD::BUILD_PAIR, DL, MVT::i64,
diff --git a/llvm/test/CodeGen/RISCV/rvv/rv32-zve-bitcast-crash.ll b/llvm/test/CodeGen/RISCV/rvv/rv32-zve-bitcast-crash.ll
new file mode 100644
index 00000000000000..0749aab9b5e80d
--- /dev/null
+++ b/llvm/test/CodeGen/RISCV/rvv/rv32-zve-bitcast-crash.ll
@@ -0,0 +1,24 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 4
+; RUN: llc < %s -mtriple=riscv32 -mattr=+zve32f,+zvl128b | FileCheck %s
+
+; This bitcast previously incorrectly produce a SplitF64 node.
+
+define i64 @foo(double %x) {
+; CHECK-LABEL: foo:
+; CHECK:       # %bb.0:
+; CHECK-NEXT:    addi sp, sp, -16
+; CHECK-NEXT:    .cfi_def_cfa_offset 16
+; CHECK-NEXT:    sw ra, 12(sp) # 4-byte Folded Spill
+; CHECK-NEXT:    .cfi_offset ra, -4
+; CHECK-NEXT:    lui a3, 261888
+; CHECK-NEXT:    li a2, 0
+; CHECK-NEXT:    call __adddf3
+; CHECK-NEXT:    lw ra, 12(sp) # 4-byte Folded Reload
+; CHECK-NEXT:    .cfi_restore ra
+; CHECK-NEXT:    addi sp, sp, 16
+; CHECK-NEXT:    .cfi_def_cfa_offset 0
+; CHECK-NEXT:    ret
+  %a = fadd double %x, 1.0
+  %b = bitcast double %a to i64
+  ret i64 %b
+}

Copy link
Contributor

@wangpc-pp wangpc-pp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
This fixes an existing bug reported by user.

Copy link
Member

@lenary lenary left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@topperc
Copy link
Collaborator

topperc commented Jan 2, 2025

Looks like the lit test didn't cherry-pick cleanly

@topperc
Copy link
Collaborator

topperc commented Jan 2, 2025

Going to manually cherry-pick to fix the test checks.

@topperc topperc closed this Jan 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Development

Successfully merging this pull request may close these issues.

4 participants