[RISCV] Add Zilsd to RISCVMergeBaseOffset.

[topperc]

I only tested a simple case for folding the addi from medany codemodel. I assume everything else should just work.

[llvmbot]

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

Author: Craig Topper (topperc)

Changes

I only tested a simple case for folding the addi from medany codemodel. I assume everything else should just work.

---

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

2 Files Affected:

• (modified) llvm/lib/Target/RISCV/RISCVMergeBaseOffset.cpp (+2)
• (added) llvm/test/CodeGen/RISCV/fold-addi-loadstore-zilsd.ll (+30)

diff --git a/llvm/lib/Target/RISCV/RISCVMergeBaseOffset.cpp b/llvm/lib/Target/RISCV/RISCVMergeBaseOffset.cpp
index eb3d43c9af7c2..60ebd0fdff2a8 100644
--- a/llvm/lib/Target/RISCV/RISCVMergeBaseOffset.cpp
+++ b/llvm/lib/Target/RISCV/RISCVMergeBaseOffset.cpp
@@ -409,6 +409,7 @@ bool RISCVMergeBaseOffsetOpt::foldIntoMemoryOps(MachineInstr &Hi,
     case RISCV::LHU:
     case RISCV::LWU:
     case RISCV::LD:
+    case RISCV::LD_RV32:
     case RISCV::FLH:
     case RISCV::FLW:
     case RISCV::FLD:
@@ -418,6 +419,7 @@ bool RISCVMergeBaseOffsetOpt::foldIntoMemoryOps(MachineInstr &Hi,
     case RISCV::SW:
     case RISCV::SW_INX:
     case RISCV::SD:
+    case RISCV::SD_RV32:
     case RISCV::FSH:
     case RISCV::FSW:
     case RISCV::FSD: {
diff --git a/llvm/test/CodeGen/RISCV/fold-addi-loadstore-zilsd.ll b/llvm/test/CodeGen/RISCV/fold-addi-loadstore-zilsd.ll
new file mode 100644
index 0000000000000..e34c5272ebaeb
--- /dev/null
+++ b/llvm/test/CodeGen/RISCV/fold-addi-loadstore-zilsd.ll
@@ -0,0 +1,30 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -mtriple=riscv32 -mattr=+zdinx,+zilsd -verify-machineinstrs \
+; RUN:   -code-model=medium < %s | FileCheck %s
+
+@g_0 = global double 0.0
+
+define double @load_g_0() nounwind {
+; CHECK-LABEL: load_g_0:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:  .Lpcrel_hi0:
+; CHECK-NEXT:    auipc a0, %pcrel_hi(g_0)
+; CHECK-NEXT:    ld a0, %pcrel_lo(.Lpcrel_hi0)(a0)
+; CHECK-NEXT:    ret
+entry:
+  %0 = load double, ptr @g_0
+  ret double %0
+}
+
+define void @store_g_0() nounwind {
+; CHECK-LABEL: store_g_0:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:  .Lpcrel_hi1:
+; CHECK-NEXT:    auipc a0, %pcrel_hi(g_0)
+; CHECK-NEXT:    fcvt.d.w a2, zero
+; CHECK-NEXT:    sd a2, %pcrel_lo(.Lpcrel_hi1)(a0)
+; CHECK-NEXT:    ret
+entry:
+  store double 0.0, ptr @g_0
+  ret void
+}

[lenary]

I think this makes sense, though I'm not super familiar with the MergeBaseOffset pass. I presume we don't have to check for any aliasing between pairs and the address-generation register.

[topperc]

I think this makes sense, though I'm not super familiar with the MergeBaseOffset pass. I presume we don't have to check for any aliasing between pairs and the address-generation register.

No, we absolutely do need to check that for stores. We have this line, but it won't work for pairs.

      // Register defined by Lo should not be the value register.
      if (DestReg == UseMI.getOperand(0).getReg())                               
        return false;

Thanks!

[topperc]

I think this makes sense, though I'm not super familiar with the MergeBaseOffset pass. I presume we don't have to check for any aliasing between pairs and the address-generation register.

No, we absolutely do need to check that for stores. We have this line, but it won't work for pairs.

      // Register defined by Lo should not be the value register.
      if (DestReg == UseMI.getOperand(0).getReg())                               
        return false;

Thanks!

Though I guess we'd have a INSERT_SUBREG or something? Not a direct use of the same virtual reg. So maybe it's ok?

[lenary]

@topperc i guess if this pass is early enough we'd have an insert_subreg. You're right that the check you pointed out is the one that I wondered if it needed generalising to some kind of "do these two alias". My main thought is that if this was a problem, we'd have already seen it with rv32Zdinx loads and stores, which i think are already supported.

[topperc]

@topperc i guess if this pass is early enough we'd have an insert_subreg. You're right that the check you pointed out is the one that I wondered if it needed generalising to some kind of "do these two alias". My main thought is that if this was a problem, we'd have already seen it with rv32Zdinx loads and stores, which i think are already supported.

rv32zdinx doesn't have any loads/stores other than the Pseudo that we expand later. And those pseudos are not recognized by this pass. And we stopped using those pseudos in isel earlier this week.