[PATCH] [RISCV] Enable -msave-restore by default when optimizing for size

[joshua-arch1]

Summary

Since spill and restore code of callee-saved registers has been implemented by common functions to reduce code duplication in the patch https://reviews.llvm.org/D62686, we can enable -msave-restore by default when we are optimizing for size in Oz.

[• pcwang-thead]

llvm/lib/Target/RISCV/RISCVMachineFunctionInfo.h

117

Use hasMinSize() if we want this feature enabled in -Oz.

[joshua-arch1]

Ping.

[hiraditya]

Can we land this patch. I think this is quite useful at -Oz

[craig.topper]

Do we still want -mno-save-restore to override this? If so maybe we need to do this in clang driver instead.

[asb]

In D134129#4156835, @craig.topper wrote:

Do we still want -mno-save-restore to override this? If so maybe we need to do this in clang driver instead.

FWIW, I think retaining the ability to have -mno-save-restore take effect is useful. So doing this in the clang driver as suggested makes sense.

[MaskRay]

At least with that this diff shows, the saving is small. I'd hope that we don't toggle -msave-restore for -Os/-Oz. Users can specify -msave-restore themselves.

[jrtc27]

In D134129#4179673, @MaskRay wrote:

At least with that this diff shows, the saving is small. I'd hope that we don't toggle -msave-restore for -Os/-Oz. Users can specify -msave-restore themselves.

Well, tiny tests like this aren't representative of real-world code, so I don't think it's fair to base it off this. Don't know what the figures are for real-world code.

llvm/test/CodeGen/RISCV/saverestore.ll

43–72

[craig.topper]

Would this break shadow call stack which isn't compatible with save/restore lib calls? Or do we already check that somewhere?

[hiraditya]

Platforms like Android reserve a register x18(for AArch64). There was a patch for RISC-V that also reserved x18 for shadow callstack. https://reviews.llvm.org/D14599, https://reviews.llvm.org/D143355

Will it be better to reserve a register that doesn't conflict with -msave-restore?

[hiraditya]

In D134129#4191314, @craig.topper wrote:

Would this break shadow call stack which isn't compatible with save/restore lib calls? Or do we already check that somewhere?

Will this still be the issue when X3 will be used for SCS?

[craig.topper]

In D134129#4326811, @hiraditya wrote:

In D134129#4191314, @craig.topper wrote:

Would this break shadow call stack which isn't compatible with save/restore lib calls? Or do we already check that somewhere?

Will this still be the issue when X3 will be used for SCS?

I don't think so as long as the error about mixing -msave-restore with SCS was removed from RISCVFrameLowering.cpp when we switched to X3.

[hiraditya]

In D134129#4326819, @craig.topper wrote:

In D134129#4326811, @hiraditya wrote:

In D134129#4191314, @craig.topper wrote:

Would this break shadow call stack which isn't compatible with save/restore lib calls? Or do we already check that somewhere?

Will this still be the issue when X3 will be used for SCS?

I don't think so as long as the error about mixing -msave-restore with SCS was removed from RISCVFrameLowering.cpp when we switched to X3.

Seems like https://reviews.llvm.org/D149099 fixed it? cc: @paulkirth for confirmation.

[paulkirth]

I don't think there is an issue anymore. As @craig.topper pointed out the check in FrameLowering is no longer there, so AFAIK there is no longer a conflict.

[hiraditya]

should we resurrect this patch?

llvm/lib/Target/RISCV/RISCVMachineFunctionInfo.h

public:
 
 
   unsigned getLibCallStackSize() const { return LibCallStackSize; }
   void setLibCallStackSize(unsigned Size) { LibCallStackSize = Size; }
 
   bool useSaveRestoreLibCalls(const MachineFunction &MF) const {
     // We cannot use fixed locations for the callee saved spill slots if the
     // function uses a varargs save area, or is an interrupt handler.
-    return MF.getSubtarget<RISCVSubtarget>().enableSaveRestore() &&
+    return (MF.getSubtarget<RISCVSubtarget>().enableSaveRestore() ||
+            MF.getFunction().hasMinSize()) &&
            VarArgsSaveSize == 0 && !MF.getFrameInfo().hasTailCall() &&
            !MF.getFunction().hasFnAttribute("interrupt");
   }
 
   uint64_t getRVVStackSize() const { return RVVStackSize; }
   void setRVVStackSize(uint64_t Size) { RVVStackSize = Size; }
 
   Align getRVVStackAlign() const { return RVVStackAlign; }

llvm/test/CodeGen/RISCV/machine-outliner-throw.ll

 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
 ; RUN: llc -verify-machineinstrs -enable-machine-outliner -mattr=+m -mtriple=riscv64 < %s | FileCheck %s
 
 ; Ensure that we won't outline CFIs when they are needed in unwinding.
 
 define i32 @func1(i32 %x) #0 {
 ; CHECK-LABEL: func1:
 ; CHECK:       # %bb.0: # %entry
-; CHECK-NEXT:    addi sp, sp, -16
+; CHECK-NEXT:    call t0, __riscv_save_1
 ; CHECK-NEXT:    .cfi_def_cfa_offset 16
-; CHECK-NEXT:    sd ra, 8(sp) # 8-byte Folded Spill
-; CHECK-NEXT:    sd s0, 0(sp) # 8-byte Folded Spill
 ; CHECK-NEXT:    .cfi_offset ra, -8
 ; CHECK-NEXT:    .cfi_offset s0, -16
 ; CHECK-NEXT:    mulw a0, a0, a0
 ; CHECK-NEXT:    addiw s0, a0, 1
 ; CHECK-NEXT:    li a0, 4
 ; CHECK-NEXT:    call __cxa_allocate_exception@plt
 ; CHECK-NEXT:    sw s0, 0(a0)
 ; CHECK-NEXT:    lui a1, %hi(_ZTIi)
 ; CHECK-NEXT:    addi a1, a1, %lo(_ZTIi)
 ; CHECK-NEXT:    li a2, 0
 ; CHECK-NEXT:    call __cxa_throw@plt
 entry:
   %mul = mul i32 %x, %x
   %add = add i32 %mul, 1
   %exception = tail call i8* @__cxa_allocate_exception(i64 4)
   %0 = bitcast i8* %exception to i32*
   store i32 %add, i32* %0
   tail call void @__cxa_throw(i8* %exception, i8* bitcast (i8** @_ZTIi to i8*), i8* null)
   unreachable
 }
 
 define i32 @func2(i32 %x) #0 {
 ; CHECK-LABEL: func2:
 ; CHECK:       # %bb.0: # %entry
-; CHECK-NEXT:    addi sp, sp, -16
+; CHECK-NEXT:    call t0, __riscv_save_1
 ; CHECK-NEXT:    .cfi_def_cfa_offset 16
-; CHECK-NEXT:    sd ra, 8(sp) # 8-byte Folded Spill
-; CHECK-NEXT:    sd s0, 0(sp) # 8-byte Folded Spill
 ; CHECK-NEXT:    .cfi_offset ra, -8
 ; CHECK-NEXT:    .cfi_offset s0, -16
 ; CHECK-NEXT:    mulw a0, a0, a0
 ; CHECK-NEXT:    addiw s0, a0, 1
 ; CHECK-NEXT:    li a0, 4
 ; CHECK-NEXT:    call __cxa_allocate_exception@plt
 ; CHECK-NEXT:    sw s0, 0(a0)
 ; CHECK-NEXT:    lui a1, %hi(_ZTIi)

llvm/test/CodeGen/RISCV/saverestore.ll

 ; RV64I-FP-SR-LABEL: callee_saved0:
 ; RV64I-FP-SR:         call t0, __riscv_save_5
 ; RV64I-FP-SR:         tail __riscv_restore_5
   %val = load [18 x i32], [18 x i32]* @var0
   store volatile [18 x i32] %val, [18 x i32]* @var0
   ret void
 }
 
+define void @callee_saved0_z() nounwind #0 {
+; RV32I-LABEL: callee_saved0_z:
+; RV32I:     call t0, __riscv_save_5
+; RV32I:     tail __riscv_restore_5
+;
+; RV64I-LABEL: callee_saved0_z:
+; RV64I:     call t0, __riscv_save_5
+; RV64I:     tail __riscv_restore_5
+;
+; RV32I-SR-LABEL: callee_saved0_z:
+; RV32I-SR:         call t0, __riscv_save_5
+; RV32I-SR:         tail __riscv_restore_5
+;
+; RV64I-SR-LABEL: callee_saved0_z:
+; RV64I-SR:         call t0, __riscv_save_5
+; RV64I-SR:         tail __riscv_restore_5
+;
+; RV32I-FP-SR-LABEL: callee_saved0_z:
+; RV32I-FP-SR:         call t0, __riscv_save_5
+; RV32I-FP-SR:         tail __riscv_restore_5
+;
+; RV64I-FP-SR-LABEL: callee_saved0_z:
+; RV64I-FP-SR:         call t0, __riscv_save_5
+; RV64I-FP-SR:         tail __riscv_restore_5
+  %val = load [18 x i32], [18 x i32]* @var0
+  store volatile [18 x i32] %val, [18 x i32]* @var0
+  ret void
+}
+
+attributes #0 = { minsize }


+
 define void @callee_saved1() nounwind {
 ; RV32I-LABEL: callee_saved1:
 ; RV32I-NOT:     call t0, __riscv_save
 ; RV32I-NOT:     tail __riscv_restore
 ;
 ; RV64I-LABEL: callee_saved1:
 ; RV64I-NOT:     call t0, __riscv_save
 ; RV64I-NOT:     tail __riscv_restore

llvm/test/CodeGen/RISCV/shifts.ll

 ; RV64I-NEXT:    ret
   %1 = lshr i64 %a, %b
   ret i64 %1
 }
 
 define i64 @lshr64_minsize(i64 %a, i64 %b) minsize nounwind {
 ; RV32I-LABEL: lshr64_minsize:
 ; RV32I:       # %bb.0:
-; RV32I-NEXT:    addi sp, sp, -16
-; RV32I-NEXT:    sw ra, 12(sp) # 4-byte Folded Spill
+; RV32I-NEXT:    call t0, __riscv_save_0
 ; RV32I-NEXT:    call __lshrdi3@plt
-; RV32I-NEXT:    lw ra, 12(sp) # 4-byte Folded Reload
-; RV32I-NEXT:    addi sp, sp, 16
-; RV32I-NEXT:    ret
+; RV32I-NEXT:    tail __riscv_restore_0
 ;
 ; RV64I-LABEL: lshr64_minsize:
 ; RV64I:       # %bb.0:
 ; RV64I-NEXT:    srl a0, a0, a1
 ; RV64I-NEXT:    ret
   %1 = lshr i64 %a, %b
   ret i64 %1
 }

 ; RV64I-NEXT:    ret
   %1 = ashr i64 %a, %b
   ret i64 %1
 }
 
 define i64 @ashr64_minsize(i64 %a, i64 %b) minsize nounwind {
 ; RV32I-LABEL: ashr64_minsize:
 ; RV32I:       # %bb.0:
-; RV32I-NEXT:    addi sp, sp, -16
-; RV32I-NEXT:    sw ra, 12(sp) # 4-byte Folded Spill
+; RV32I-NEXT:    call t0, __riscv_save_0
 ; RV32I-NEXT:    call __ashrdi3@plt
-; RV32I-NEXT:    lw ra, 12(sp) # 4-byte Folded Reload
-; RV32I-NEXT:    addi sp, sp, 16
-; RV32I-NEXT:    ret
+; RV32I-NEXT:    tail __riscv_restore_0
 ;
 ; RV64I-LABEL: ashr64_minsize:
 ; RV64I:       # %bb.0:
 ; RV64I-NEXT:    sra a0, a0, a1
 ; RV64I-NEXT:    ret
   %1 = ashr i64 %a, %b
   ret i64 %1
 }

 ; RV64I-NEXT:    ret
   %1 = shl i64 %a, %b
   ret i64 %1
 }
 
 define i64 @shl64_minsize(i64 %a, i64 %b) minsize nounwind {
 ; RV32I-LABEL: shl64_minsize:
 ; RV32I:       # %bb.0:
-; RV32I-NEXT:    addi sp, sp, -16
-; RV32I-NEXT:    sw ra, 12(sp) # 4-byte Folded Spill
+; RV32I-NEXT:    call t0, __riscv_save_0
 ; RV32I-NEXT:    call __ashldi3@plt
-; RV32I-NEXT:    lw ra, 12(sp) # 4-byte Folded Reload
-; RV32I-NEXT:    addi sp, sp, 16
-; RV32I-NEXT:    ret
+; RV32I-NEXT:    tail __riscv_restore_0
 ;
 ; RV64I-LABEL: shl64_minsize:
 ; RV64I:       # %bb.0:
 ; RV64I-NEXT:    sll a0, a0, a1
 ; RV64I-NEXT:    ret
   %1 = shl i64 %a, %b
   ret i64 %1
 }