[RISCV] Explicitly set FRM defs as non-dead to prevent their reordering with instructions that may use it

[skachkov-sc]

Fixes #135172. The proposed solution is to conservatively reset dead flag from all $frm defs in AdjustInstrPostInstrSelection (this is probably a bit hacky, so I'm open to the better ideas)

[llvmbot]

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

Author: Sergey Kachkov (skachkov-sc)

Changes

Fixes #135172. The proposed solution is to conservatively reset dead flag from all $frm defs in AdjustInstrPostInstrSelection (this is probably a bit hacky, so I'm open to the better ideas)

---

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

3 Files Affected:

• (modified) llvm/lib/Target/RISCV/RISCVISelLowering.cpp (+9)
• (modified) llvm/lib/Target/RISCV/RISCVInstrInfo.td (+2)
• (added) llvm/test/CodeGen/RISCV/frm-write-in-loop.ll (+35)

diff --git a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
index f7d192756fd56..414fb58f0cbeb 100644
--- a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
+++ b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
@@ -20926,6 +20926,15 @@ RISCVTargetLowering::EmitInstrWithCustomInserter(MachineInstr &MI,
 
 void RISCVTargetLowering::AdjustInstrPostInstrSelection(MachineInstr &MI,
                                                         SDNode *Node) const {
+  // If instruction defines FRM operand, conservatively set it as non-dead to
+  // express data dependency with FRM users and prevent incorrect instruction
+  // reordering (these defs are marked as dead because at the moment of
+  // instruction emission they actually don't have any uses - they are added
+  // later in this hook).
+  if (auto *FRMDef = MI.findRegisterDefOperand(RISCV::FRM, /*TRI=*/nullptr)) {
+    FRMDef->setIsDead(false);
+    return;
+  }
   // Add FRM dependency to any instructions with dynamic rounding mode.
   int Idx = RISCV::getNamedOperandIdx(MI.getOpcode(), RISCV::OpName::frm);
   if (Idx < 0) {
diff --git a/llvm/lib/Target/RISCV/RISCVInstrInfo.td b/llvm/lib/Target/RISCV/RISCVInstrInfo.td
index c87452171f090..1104d9089536f 100644
--- a/llvm/lib/Target/RISCV/RISCVInstrInfo.td
+++ b/llvm/lib/Target/RISCV/RISCVInstrInfo.td
@@ -1941,9 +1941,11 @@ class SwapSysRegImm<SysReg SR, list<Register> Regs>
 }
 
 def ReadFRM : ReadSysReg<SysRegFRM, [FRM]>;
+let hasPostISelHook = 1 in {
 def WriteFRM : WriteSysReg<SysRegFRM, [FRM]>;
 def WriteFRMImm : WriteSysRegImm<SysRegFRM, [FRM]>;
 def SwapFRMImm : SwapSysRegImm<SysRegFRM, [FRM]>;
+}
 
 def WriteVXRMImm : WriteSysRegImm<SysRegVXRM, [VXRM]>;
 
diff --git a/llvm/test/CodeGen/RISCV/frm-write-in-loop.ll b/llvm/test/CodeGen/RISCV/frm-write-in-loop.ll
new file mode 100644
index 0000000000000..9ef8e4b67edd7
--- /dev/null
+++ b/llvm/test/CodeGen/RISCV/frm-write-in-loop.ll
@@ -0,0 +1,35 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 5
+; RUN: llc -O3 -mtriple=riscv64 -mattr=+f,+d < %s | FileCheck %s
+
+; Make sure WriteFRM is not hoisted out of loop due to dead implicit-def.
+
+define double @foo(double %0, double %1, i64 %n) strictfp {
+; CHECK-LABEL: foo:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    fmv.d.x fa5, zero
+; CHECK-NEXT:  .LBB0_1: # %loop
+; CHECK-NEXT:    # =>This Inner Loop Header: Depth=1
+; CHECK-NEXT:    fsrmi 3
+; CHECK-NEXT:    fadd.d fa5, fa5, fa0
+; CHECK-NEXT:    fsrmi 0
+; CHECK-NEXT:    addi a0, a0, -1
+; CHECK-NEXT:    fadd.d fa5, fa5, fa1
+; CHECK-NEXT:    beqz a0, .LBB0_1
+; CHECK-NEXT:  # %bb.2: # %exit
+; CHECK-NEXT:    fmv.d fa0, fa5
+; CHECK-NEXT:    ret
+entry:
+    br label %loop
+loop:
+    %cnt = phi i64 [0, %entry], [%cnt_inc, %loop]
+    %acc = phi double [0.0, %entry], [%f2, %loop]
+    call void @llvm.set.rounding(i32 2) strictfp
+    %f1 = call double @llvm.experimental.constrained.fadd.f64(double %acc, double %0, metadata !"round.dynamic", metadata !"fpexcept.ignore") strictfp
+    call void @llvm.set.rounding(i32 1) strictfp
+    %f2 = call double @llvm.experimental.constrained.fadd.f64(double %f1, double %1, metadata !"round.dynamic", metadata !"fpexcept.ignore") strictfp
+    %cnt_inc = add i64 %cnt, 1
+    %cond = icmp eq i64 %cnt_inc, %n
+    br i1 %cond, label %loop, label %exit
+exit:
+    ret double %f2
+}

[topperc]

I don't think the code in InstrEmitter that controls the setting of the dead flag would see them even if they were done earlier. It only checks glued nodes or explicit data dependencies. Neither apply to the input case.

[skachkov-sc]

Removed this statement to avoid confusion

[topperc]

I think we might need a bigger hammer. This case also hoists but we don't need if @foo uses FRM or changes FRM.

define double @bar(double %0, double %1, i64 %n) strictfp {                      
entry:                                                                           
    br label %loop                                                               
loop:                                                                            
    %cnt = phi i64 [0, %entry], [%cnt_inc, %loop]                                
    %acc = phi double [0.0, %entry], [%f2, %loop]                                
    call void @llvm.set.rounding(i32 2) strictfp                                 
    %f1 = call double @foo(double %acc, double %0) strictfp                      
    call void @llvm.set.rounding(i32 1) strictfp                                 
    %f2 = call double @foo(double %f1, double %1) strictfp                       
    %cnt_inc = add i64 %cnt, 1                                                   
    %cond = icmp eq i64 %cnt_inc, %n                                             
    br i1 %cond, label %loop, label %exit                                        
exit:                                                                            
    ret double %f2                                                               
}                                                                                
                                                                                 
declare double @foo(double, double)

[topperc]

I think we might need a bigger hammer. This case also hoists but we don't need if @foo uses FRM or changes FRM.

define double @bar(double %0, double %1, i64 %n) strictfp {                      
entry:                                                                           
    br label %loop                                                               
loop:                                                                            
    %cnt = phi i64 [0, %entry], [%cnt_inc, %loop]                                
    %acc = phi double [0.0, %entry], [%f2, %loop]                                
    call void @llvm.set.rounding(i32 2) strictfp                                 
    %f1 = call double @foo(double %acc, double %0) strictfp                      
    call void @llvm.set.rounding(i32 1) strictfp                                 
    %f2 = call double @foo(double %f1, double %1) strictfp                       
    %cnt_inc = add i64 %cnt, 1                                                   
    %cond = icmp eq i64 %cnt_inc, %n                                             
    br i1 %cond, label %loop, label %exit                                        
exit:                                                                            
    ret double %f2                                                               
}                                                                                
                                                                                 
declare double @foo(double, double)

Looks like this patch fixes this test case too.

[topperc]

LGTM

[skachkov-sc]

I think we might need a bigger hammer. This case also hoists but we don't need if @foo uses FRM or changes FRM.

define double @bar(double %0, double %1, i64 %n) strictfp {                      
entry:                                                                           
    br label %loop                                                               
loop:                                                                            
    %cnt = phi i64 [0, %entry], [%cnt_inc, %loop]                                
    %acc = phi double [0.0, %entry], [%f2, %loop]                                
    call void @llvm.set.rounding(i32 2) strictfp                                 
    %f1 = call double @foo(double %acc, double %0) strictfp                      
    call void @llvm.set.rounding(i32 1) strictfp                                 
    %f2 = call double @foo(double %f1, double %1) strictfp                       
    %cnt_inc = add i64 %cnt, 1                                                   
    %cond = icmp eq i64 %cnt_inc, %n                                             
    br i1 %cond, label %loop, label %exit                                        
exit:                                                                            
    ret double %f2                                                               
}                                                                                
                                                                                 
declare double @foo(double, double)

Also added this test case