This is an archive of the discontinued LLVM Phabricator instance.

Differential D124463

[RISCV] Add isCommutable to scalar FMA instructions.
ClosedPublic

Authored by craig.topper on Apr 26 2022, 9:23 AM.

Details

Reviewers
asb
luismarques
jrtc27
reames
Commits
rGc2614b31d955: [RISCV] Add isCommutable to scalar FMA instructions.
Summary

The default implementation of findCommutedOpIndices picks the
first two source operands. That's exactly what we want for the
scalar FMA instructions.

Diff Detail

Event Timeline

craig.topper created this revision.Apr 26 2022, 9:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 26 2022, 9:23 AM
Herald added subscribers: sunshaoce, VincentWu, luke957 and 26 others. · View Herald Transcript
craig.topper requested review of this revision.Apr 26 2022, 9:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 26 2022, 9:23 AM
Herald added subscribers: • pcwang-thead, eopXD, MaskRay. · View Herald Transcript
reames added a comment.Apr 26 2022, 10:27 AM

Declaring a three operand opcode as commutative when only two of the arguments can be safely commuted feels rather suspect to me.

Harbormaster completed remote builds in B161419: Diff 425245.Apr 26 2022, 10:34 AM
craig.topper added a comment.Apr 26 2022, 10:37 AM
In D124463#3475151, @reames wrote:

Declaring a three operand opcode as commutative when only two of the arguments can be safely commuted feels rather suspect to me.

The commutable operands are determined by TargetInstrInfo::findCommutedOpIndices. The default implementation picks the first two source operands. X86 relies on this for commuting AMD's FMA4 instructions. For the FMA3 instructions, X86 overrides TargetInstrInfo::findCommutedOpIndices and calls X86InstrInfo::findThreeSrcCommutedOpIndices. An additional override of TargetInstrInfo::commuteInstructionImpl is used to change the opcode for FMA3.

Should I add more tests or more comments or both?

reames accepted this revision.Apr 26 2022, 11:54 AM
In D124463#3475162, @craig.topper wrote:
In D124463#3475151, @reames wrote:

Declaring a three operand opcode as commutative when only two of the arguments can be safely commuted feels rather suspect to me.

The commutable operands are determined by TargetInstrInfo::findCommutedOpIndices. The default implementation picks the first two source operands. X86 relies on this for commuting AMD's FMA4 instructions. For the FMA3 instructions, X86 overrides TargetInstrInfo::findCommutedOpIndices and calls X86InstrInfo::findThreeSrcCommutedOpIndices. An additional override of TargetInstrInfo::commuteInstructionImpl is used to change the opcode for FMA3.

Should I add more tests or more comments or both?

I'm not arguing correctness of the change, just that it is quite confusingly structured. However, I went and dug through the existing code as you suggested, and you're definitely right that backends rely on the first-two-non-def-operands thing already. This isn't adding new weirdness, it's just matching what everyone else already does.

Given that, LGTM.

This revision is now accepted and ready to land.Apr 26 2022, 11:54 AM
This revision was landed with ongoing or failed builds.Apr 27 2022, 11:17 AM
Closed by commit rGc2614b31d955: [RISCV] Add isCommutable to scalar FMA instructions. (authored by craig.topper). · Explain Why
This revision was automatically updated to reflect the committed changes.
craig.topper added a commit: rGc2614b31d955: [RISCV] Add isCommutable to scalar FMA instructions..

Revision Contents

PathSize
llvm/
lib/
Target/
RISCV/
2 lines
test/
CodeGen/
RISCV/
426 lines

Diff 425245

llvm/lib/Target/RISCV/RISCVInstrInfoF.td

Show First 20 Lines • Show All 153 Lines • ▼ Show 20 Lines
class FPStore_r<bits<3> funct3, string opcodestr, RegisterClass rty, class FPStore_r<bits<3> funct3, string opcodestr, RegisterClass rty,
SchedWrite sw> SchedWrite sw>
: RVInstS<funct3, OPC_STORE_FP, (outs), : RVInstS<funct3, OPC_STORE_FP, (outs),
(ins rty:$rs2, GPR:$rs1, simm12:$imm12), (ins rty:$rs2, GPR:$rs1, simm12:$imm12),
opcodestr, "$rs2, ${imm12}(${rs1})">, opcodestr, "$rs2, ${imm12}(${rs1})">,
Sched<[sw, ReadStoreData, ReadFMemBase]>; Sched<[sw, ReadStoreData, ReadFMemBase]>;
let hasSideEffects = 0, mayLoad = 0, mayStore = 0, mayRaiseFPException = 1, let hasSideEffects = 0, mayLoad = 0, mayStore = 0, mayRaiseFPException = 1,
UseNamedOperandTable = 1, hasPostISelHook = 1 in UseNamedOperandTable = 1, hasPostISelHook = 1, isCommutable = 1 in
class FPFMA_rrr_frm<RISCVOpcode opcode, bits<2> funct2, string opcodestr, class FPFMA_rrr_frm<RISCVOpcode opcode, bits<2> funct2, string opcodestr,
DAGOperand rty> DAGOperand rty>
: RVInstR4Frm<funct2, opcode, (outs rty:$rd), : RVInstR4Frm<funct2, opcode, (outs rty:$rd),
(ins rty:$rs1, rty:$rs2, rty:$rs3, frmarg:$frm), (ins rty:$rs1, rty:$rs2, rty:$rs3, frmarg:$frm),
opcodestr, "$rd, $rs1, $rs2, $rs3, $frm">; opcodestr, "$rd, $rs1, $rs2, $rs3, $frm">;
multiclass FPFMA_rrr_frm_m<RISCVOpcode opcode, bits<2> funct2, multiclass FPFMA_rrr_frm_m<RISCVOpcode opcode, bits<2> funct2,
string opcodestr, list<ExtInfo_r> Exts> { string opcodestr, list<ExtInfo_r> Exts> {
▲ Show 20 Lines • Show All 459 Lines • Show Last 20 Lines

llvm/test/CodeGen/RISCV/machine-cse.ll

Show First 20 Lines • Show All 276 Lines • ▼ Show 20 Linestrueblock:
%c = fcmp oeq double %y, %x %c = fcmp oeq double %y, %x
%d = zext i1 %c to i8 %d = zext i1 %c to i8
store i8 %d, i8* %p1 store i8 %d, i8* %p1
br label %falseblock br label %falseblock
falseblock: falseblock:
ret void ret void
} }
declare half @llvm.fma.f16(half, half, half)
define void @commute_fmadd_f16(half %x, half %y, half %z, half* %p1, half* %p2, i1 zeroext %cond) {
; RV32-LABEL: commute_fmadd_f16:
; RV32: # %bb.0:
; RV32-NEXT: fmadd.h ft0, fa0, fa1, fa2
; RV32-NEXT: fsh ft0, 0(a0)
; RV32-NEXT: beqz a2, .LBB8_2
; RV32-NEXT: # %bb.1: # %trueblock
; RV32-NEXT: fsh ft0, 0(a0)
; RV32-NEXT: .LBB8_2: # %falseblock
; RV32-NEXT: ret
;
; RV64-LABEL: commute_fmadd_f16:
; RV64: # %bb.0:
; RV64-NEXT: fmadd.h ft0, fa0, fa1, fa2
; RV64-NEXT: fsh ft0, 0(a0)
; RV64-NEXT: beqz a2, .LBB8_2
; RV64-NEXT: # %bb.1: # %trueblock
; RV64-NEXT: fsh ft0, 0(a0)
; RV64-NEXT: .LBB8_2: # %falseblock
; RV64-NEXT: ret
%a = call half @llvm.fma.f16(half %x, half %y, half %z)
store half %a, half* %p1
br i1 %cond, label %trueblock, label %falseblock
trueblock:
%b = call half @llvm.fma.f16(half %y, half %x, half %z)
store half %b, half* %p1
br label %falseblock
falseblock:
ret void
}
declare float @llvm.fma.f32(float, float, float)
define void @commute_fmadd_f32(float %x, float %y, float %z, float* %p1, float* %p2, i1 zeroext %cond) {
; RV32-LABEL: commute_fmadd_f32:
; RV32: # %bb.0:
; RV32-NEXT: fmadd.s ft0, fa0, fa1, fa2
; RV32-NEXT: fsw ft0, 0(a0)
; RV32-NEXT: beqz a2, .LBB9_2
; RV32-NEXT: # %bb.1: # %trueblock
; RV32-NEXT: fsw ft0, 0(a0)
; RV32-NEXT: .LBB9_2: # %falseblock
; RV32-NEXT: ret
;
; RV64-LABEL: commute_fmadd_f32:
; RV64: # %bb.0:
; RV64-NEXT: fmadd.s ft0, fa0, fa1, fa2
; RV64-NEXT: fsw ft0, 0(a0)
; RV64-NEXT: beqz a2, .LBB9_2
; RV64-NEXT: # %bb.1: # %trueblock
; RV64-NEXT: fsw ft0, 0(a0)
; RV64-NEXT: .LBB9_2: # %falseblock
; RV64-NEXT: ret
%a = call float @llvm.fma.f32(float %x, float %y, float %z)
store float %a, float* %p1
br i1 %cond, label %trueblock, label %falseblock
trueblock:
%b = call float @llvm.fma.f32(float %y, float %x, float %z)
store float %b, float* %p1
br label %falseblock
falseblock:
ret void
}
declare double @llvm.fma.f64(double, double, double)
define void @commute_fmadd_f64(double %x, double %y, double %z, double* %p1, double* %p2, i1 zeroext %cond) {
; RV32-LABEL: commute_fmadd_f64:
; RV32: # %bb.0:
; RV32-NEXT: fmadd.d ft0, fa0, fa1, fa2
; RV32-NEXT: fsd ft0, 0(a0)
; RV32-NEXT: beqz a2, .LBB10_2
; RV32-NEXT: # %bb.1: # %trueblock
; RV32-NEXT: fsd ft0, 0(a0)
; RV32-NEXT: .LBB10_2: # %falseblock
; RV32-NEXT: ret
;
; RV64-LABEL: commute_fmadd_f64:
; RV64: # %bb.0:
; RV64-NEXT: fmadd.d ft0, fa0, fa1, fa2
; RV64-NEXT: fsd ft0, 0(a0)
; RV64-NEXT: beqz a2, .LBB10_2
; RV64-NEXT: # %bb.1: # %trueblock
; RV64-NEXT: fsd ft0, 0(a0)
; RV64-NEXT: .LBB10_2: # %falseblock
; RV64-NEXT: ret
%a = call double @llvm.fma.f64(double %x, double %y, double %z)
store double %a, double* %p1
br i1 %cond, label %trueblock, label %falseblock
trueblock:
%b = call double @llvm.fma.f64(double %y, double %x, double %z)
store double %b, double* %p1
br label %falseblock
falseblock:
ret void
}
define void @commute_fmsub_f16(half %x, half %y, half %z, half* %p1, half* %p2, i1 zeroext %cond) {
; RV32-LABEL: commute_fmsub_f16:
; RV32: # %bb.0:
; RV32-NEXT: fmsub.h ft0, fa0, fa1, fa2
; RV32-NEXT: fsh ft0, 0(a0)
; RV32-NEXT: beqz a2, .LBB11_2
; RV32-NEXT: # %bb.1: # %trueblock
; RV32-NEXT: fsh ft0, 0(a0)
; RV32-NEXT: .LBB11_2: # %falseblock
; RV32-NEXT: ret
;
; RV64-LABEL: commute_fmsub_f16:
; RV64: # %bb.0:
; RV64-NEXT: fmsub.h ft0, fa0, fa1, fa2
; RV64-NEXT: fsh ft0, 0(a0)
; RV64-NEXT: beqz a2, .LBB11_2
; RV64-NEXT: # %bb.1: # %trueblock
; RV64-NEXT: fsh ft0, 0(a0)
; RV64-NEXT: .LBB11_2: # %falseblock
; RV64-NEXT: ret
%negz = fneg half %z
%a = call half @llvm.fma.f16(half %x, half %y, half %negz)
store half %a, half* %p1
br i1 %cond, label %trueblock, label %falseblock
trueblock:
%negz2 = fneg half %z
%b = call half @llvm.fma.f16(half %y, half %x, half %negz2)
store half %b, half* %p1
br label %falseblock
falseblock:
ret void
}
define void @commute_fmsub_f32(float %x, float %y, float %z, float* %p1, float* %p2, i1 zeroext %cond) {
; RV32-LABEL: commute_fmsub_f32:
; RV32: # %bb.0:
; RV32-NEXT: fmsub.s ft0, fa0, fa1, fa2
; RV32-NEXT: fsw ft0, 0(a0)
; RV32-NEXT: beqz a2, .LBB12_2
; RV32-NEXT: # %bb.1: # %trueblock
; RV32-NEXT: fsw ft0, 0(a0)
; RV32-NEXT: .LBB12_2: # %falseblock
; RV32-NEXT: ret
;
; RV64-LABEL: commute_fmsub_f32:
; RV64: # %bb.0:
; RV64-NEXT: fmsub.s ft0, fa0, fa1, fa2
; RV64-NEXT: fsw ft0, 0(a0)
; RV64-NEXT: beqz a2, .LBB12_2
; RV64-NEXT: # %bb.1: # %trueblock
; RV64-NEXT: fsw ft0, 0(a0)
; RV64-NEXT: .LBB12_2: # %falseblock
; RV64-NEXT: ret
%negz = fneg float %z
%a = call float @llvm.fma.f32(float %x, float %y, float %negz)
store float %a, float* %p1
br i1 %cond, label %trueblock, label %falseblock
trueblock:
%negz2 = fneg float %z
%b = call float @llvm.fma.f32(float %y, float %x, float %negz2)
store float %b, float* %p1
br label %falseblock
falseblock:
ret void
}
define void @commute_fmsub_f64(double %x, double %y, double %z, double* %p1, double* %p2, i1 zeroext %cond) {
; RV32-LABEL: commute_fmsub_f64:
; RV32: # %bb.0:
; RV32-NEXT: fmsub.d ft0, fa0, fa1, fa2
; RV32-NEXT: fsd ft0, 0(a0)
; RV32-NEXT: beqz a2, .LBB13_2
; RV32-NEXT: # %bb.1: # %trueblock
; RV32-NEXT: fsd ft0, 0(a0)
; RV32-NEXT: .LBB13_2: # %falseblock
; RV32-NEXT: ret
;
; RV64-LABEL: commute_fmsub_f64:
; RV64: # %bb.0:
; RV64-NEXT: fmsub.d ft0, fa0, fa1, fa2
; RV64-NEXT: fsd ft0, 0(a0)
; RV64-NEXT: beqz a2, .LBB13_2
; RV64-NEXT: # %bb.1: # %trueblock
; RV64-NEXT: fsd ft0, 0(a0)
; RV64-NEXT: .LBB13_2: # %falseblock
; RV64-NEXT: ret
%negz = fneg double %z
%a = call double @llvm.fma.f64(double %x, double %y, double %negz)
store double %a, double* %p1
br i1 %cond, label %trueblock, label %falseblock
trueblock:
%negz2 = fneg double %z
%b = call double @llvm.fma.f64(double %y, double %x, double %negz2)
store double %b, double* %p1
br label %falseblock
falseblock:
ret void
}
define void @commute_fnmadd_f16(half %x, half %y, half %z, half* %p1, half* %p2, i1 zeroext %cond) {
; RV32-LABEL: commute_fnmadd_f16:
; RV32: # %bb.0:
; RV32-NEXT: fnmadd.h ft0, fa0, fa1, fa2
; RV32-NEXT: fsh ft0, 0(a0)
; RV32-NEXT: beqz a2, .LBB14_2
; RV32-NEXT: # %bb.1: # %trueblock
; RV32-NEXT: fsh ft0, 0(a0)
; RV32-NEXT: .LBB14_2: # %falseblock
; RV32-NEXT: ret
;
; RV64-LABEL: commute_fnmadd_f16:
; RV64: # %bb.0:
; RV64-NEXT: fnmadd.h ft0, fa0, fa1, fa2
; RV64-NEXT: fsh ft0, 0(a0)
; RV64-NEXT: beqz a2, .LBB14_2
; RV64-NEXT: # %bb.1: # %trueblock
; RV64-NEXT: fsh ft0, 0(a0)
; RV64-NEXT: .LBB14_2: # %falseblock
; RV64-NEXT: ret
%negx = fneg half %x
%negz = fneg half %z
%a = call half @llvm.fma.f16(half %negx, half %y, half %negz)
store half %a, half* %p1
br i1 %cond, label %trueblock, label %falseblock
trueblock:
%negy = fneg half %y
%negz2 = fneg half %z
%b = call half @llvm.fma.f16(half %negy, half %x, half %negz2)
store half %b, half* %p1
br label %falseblock
falseblock:
ret void
}
define void @commute_fnmadd_f32(float %x, float %y, float %z, float* %p1, float* %p2, i1 zeroext %cond) {
; RV32-LABEL: commute_fnmadd_f32:
; RV32: # %bb.0:
; RV32-NEXT: fnmadd.s ft0, fa0, fa1, fa2
; RV32-NEXT: fsw ft0, 0(a0)
; RV32-NEXT: beqz a2, .LBB15_2
; RV32-NEXT: # %bb.1: # %trueblock
; RV32-NEXT: fsw ft0, 0(a0)
; RV32-NEXT: .LBB15_2: # %falseblock
; RV32-NEXT: ret
;
; RV64-LABEL: commute_fnmadd_f32:
; RV64: # %bb.0:
; RV64-NEXT: fnmadd.s ft0, fa0, fa1, fa2
; RV64-NEXT: fsw ft0, 0(a0)
; RV64-NEXT: beqz a2, .LBB15_2
; RV64-NEXT: # %bb.1: # %trueblock
; RV64-NEXT: fsw ft0, 0(a0)
; RV64-NEXT: .LBB15_2: # %falseblock
; RV64-NEXT: ret
%negx = fneg float %x
%negz = fneg float %z
%a = call float @llvm.fma.f32(float %negx, float %y, float %negz)
store float %a, float* %p1
br i1 %cond, label %trueblock, label %falseblock
trueblock:
%negy = fneg float %y
%negz2 = fneg float %z
%b = call float @llvm.fma.f32(float %negy, float %x, float %negz2)
store float %b, float* %p1
br label %falseblock
falseblock:
ret void
}
define void @commute_fnmadd_f64(double %x, double %y, double %z, double* %p1, double* %p2, i1 zeroext %cond) {
; RV32-LABEL: commute_fnmadd_f64:
; RV32: # %bb.0:
; RV32-NEXT: fnmadd.d ft0, fa0, fa1, fa2
; RV32-NEXT: fsd ft0, 0(a0)
; RV32-NEXT: beqz a2, .LBB16_2
; RV32-NEXT: # %bb.1: # %trueblock
; RV32-NEXT: fsd ft0, 0(a0)
; RV32-NEXT: .LBB16_2: # %falseblock
; RV32-NEXT: ret
;
; RV64-LABEL: commute_fnmadd_f64:
; RV64: # %bb.0:
; RV64-NEXT: fnmadd.d ft0, fa0, fa1, fa2
; RV64-NEXT: fsd ft0, 0(a0)
; RV64-NEXT: beqz a2, .LBB16_2
; RV64-NEXT: # %bb.1: # %trueblock
; RV64-NEXT: fsd ft0, 0(a0)
; RV64-NEXT: .LBB16_2: # %falseblock
; RV64-NEXT: ret
%negx = fneg double %x
%negz = fneg double %z
%a = call double @llvm.fma.f64(double %negx, double %y, double %negz)
store double %a, double* %p1
br i1 %cond, label %trueblock, label %falseblock
trueblock:
%negy = fneg double %y
%negz2 = fneg double %z
%b = call double @llvm.fma.f64(double %negy, double %x, double %negz2)
store double %b, double* %p1
br label %falseblock
falseblock:
ret void
}
define void @commute_fnmsub_f16(half %x, half %y, half %z, half* %p1, half* %p2, i1 zeroext %cond) {
; RV32-LABEL: commute_fnmsub_f16:
; RV32: # %bb.0:
; RV32-NEXT: fnmsub.h ft0, fa0, fa1, fa2
; RV32-NEXT: fsh ft0, 0(a0)
; RV32-NEXT: beqz a2, .LBB17_2
; RV32-NEXT: # %bb.1: # %trueblock
; RV32-NEXT: fsh ft0, 0(a0)
; RV32-NEXT: .LBB17_2: # %falseblock
; RV32-NEXT: ret
;
; RV64-LABEL: commute_fnmsub_f16:
; RV64: # %bb.0:
; RV64-NEXT: fnmsub.h ft0, fa0, fa1, fa2
; RV64-NEXT: fsh ft0, 0(a0)
; RV64-NEXT: beqz a2, .LBB17_2
; RV64-NEXT: # %bb.1: # %trueblock
; RV64-NEXT: fsh ft0, 0(a0)
; RV64-NEXT: .LBB17_2: # %falseblock
; RV64-NEXT: ret
%negx = fneg half %x
%a = call half @llvm.fma.f16(half %negx, half %y, half %z)
store half %a, half* %p1
br i1 %cond, label %trueblock, label %falseblock
trueblock:
%negy = fneg half %y
%b = call half @llvm.fma.f16(half %negy, half %x, half %z)
store half %b, half* %p1
br label %falseblock
falseblock:
ret void
}
define void @commute_fnmsub_f32(float %x, float %y, float %z, float* %p1, float* %p2, i1 zeroext %cond) {
; RV32-LABEL: commute_fnmsub_f32:
; RV32: # %bb.0:
; RV32-NEXT: fnmsub.s ft0, fa0, fa1, fa2
; RV32-NEXT: fsw ft0, 0(a0)
; RV32-NEXT: beqz a2, .LBB18_2
; RV32-NEXT: # %bb.1: # %trueblock
; RV32-NEXT: fsw ft0, 0(a0)
; RV32-NEXT: .LBB18_2: # %falseblock
; RV32-NEXT: ret
;
; RV64-LABEL: commute_fnmsub_f32:
; RV64: # %bb.0:
; RV64-NEXT: fnmsub.s ft0, fa0, fa1, fa2
; RV64-NEXT: fsw ft0, 0(a0)
; RV64-NEXT: beqz a2, .LBB18_2
; RV64-NEXT: # %bb.1: # %trueblock
; RV64-NEXT: fsw ft0, 0(a0)
; RV64-NEXT: .LBB18_2: # %falseblock
; RV64-NEXT: ret
%negx = fneg float %x
%a = call float @llvm.fma.f32(float %negx, float %y, float %z)
store float %a, float* %p1
br i1 %cond, label %trueblock, label %falseblock
trueblock:
%negy = fneg float %y
%b = call float @llvm.fma.f32(float %negy, float %x, float %z)
store float %b, float* %p1
br label %falseblock
falseblock:
ret void
}
define void @commute_fnmsub_f64(double %x, double %y, double %z, double* %p1, double* %p2, i1 zeroext %cond) {
; RV32-LABEL: commute_fnmsub_f64:
; RV32: # %bb.0:
; RV32-NEXT: fnmsub.d ft0, fa0, fa1, fa2
; RV32-NEXT: fsd ft0, 0(a0)
; RV32-NEXT: beqz a2, .LBB19_2
; RV32-NEXT: # %bb.1: # %trueblock
; RV32-NEXT: fsd ft0, 0(a0)
; RV32-NEXT: .LBB19_2: # %falseblock
; RV32-NEXT: ret
;
; RV64-LABEL: commute_fnmsub_f64:
; RV64: # %bb.0:
; RV64-NEXT: fnmsub.d ft0, fa0, fa1, fa2
; RV64-NEXT: fsd ft0, 0(a0)
; RV64-NEXT: beqz a2, .LBB19_2
; RV64-NEXT: # %bb.1: # %trueblock
; RV64-NEXT: fsd ft0, 0(a0)
; RV64-NEXT: .LBB19_2: # %falseblock
; RV64-NEXT: ret
%negx = fneg double %x
%a = call double @llvm.fma.f64(double %negx, double %y, double %z)
store double %a, double* %p1
br i1 %cond, label %trueblock, label %falseblock
trueblock:
%negy = fneg double %y
%b = call double @llvm.fma.f64(double %negy, double %x, double %z)
store double %b, double* %p1
br label %falseblock
falseblock:
ret void
}