[PATCH] D72820: [FPEnv] Add pragma FP_CONTRACT support under strict FP.
pengfei added inline comments. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3386 +FMulAdd = Builder.CreateConstrainedFPCall( +CGF.CGM.getIntrinsic(llvm::Intrinsic::experimental_constrained_fmuladd, + Addend->getType()), Allen wrote: > Sorry, I'm not familiar with the optimization of the clang front end. > I'd like to ask, is this optimization supposed to assume that all the > backends have instructions like Fmuladd? No, it is a flexible intrinsic, which allows backends to choose their best approach. It can be either interpretered as mul + add or fma. It represents user doesn't care the differece between them. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72820/new/ https://reviews.llvm.org/D72820 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D72820: [FPEnv] Add pragma FP_CONTRACT support under strict FP.
Allen added inline comments. Herald added a project: All. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3386 +FMulAdd = Builder.CreateConstrainedFPCall( +CGF.CGM.getIntrinsic(llvm::Intrinsic::experimental_constrained_fmuladd, + Addend->getType()), Sorry, I'm not familiar with the optimization of the clang front end. I'd like to ask, is this optimization supposed to assume that all the backends have instructions like Fmuladd? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72820/new/ https://reviews.llvm.org/D72820 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D72820: [FPEnv] Add pragma FP_CONTRACT support under strict FP.
pengfei marked an inline comment as done. pengfei added inline comments. Comment at: llvm/docs/LangRef.rst:16145 +'``llvm.experimental.constrained.fmuladd``' Intrinsic +^^^ + jhenderson wrote: > This underline isn't long enough and is breaking the sphinx build bot. Please > fix. Thanks! I'll fix it soon. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72820/new/ https://reviews.llvm.org/D72820 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D72820: [FPEnv] Add pragma FP_CONTRACT support under strict FP.
jhenderson added inline comments. Comment at: llvm/docs/LangRef.rst:16145 +'``llvm.experimental.constrained.fmuladd``' Intrinsic +^^^ + This underline isn't long enough and is breaking the sphinx build bot. Please fix. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72820/new/ https://reviews.llvm.org/D72820 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D72820: [FPEnv] Add pragma FP_CONTRACT support under strict FP.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG3239b5034ee9: [FPEnv] Add pragma FP_CONTRACT support under
strict FP. (authored by pengfei).
Changed prior to commit:
https://reviews.llvm.org/D72820?vs=240474=240840#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D72820/new/
https://reviews.llvm.org/D72820
Files:
clang/lib/CodeGen/CGExprScalar.cpp
clang/test/CodeGen/constrained-math-builtins.c
llvm/docs/LangRef.rst
llvm/include/llvm/CodeGen/BasicTTIImpl.h
llvm/include/llvm/IR/ConstrainedOps.def
llvm/include/llvm/IR/Intrinsics.td
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
llvm/test/CodeGen/X86/fp-intrinsics-fma.ll
Index: llvm/test/CodeGen/X86/fp-intrinsics-fma.ll
===
--- llvm/test/CodeGen/X86/fp-intrinsics-fma.ll
+++ llvm/test/CodeGen/X86/fp-intrinsics-fma.ll
@@ -322,6 +322,128 @@
ret double %result
}
+; Verify constrained fmul and fadd aren't fused.
+define float @f11(float %0, float %1, float %2) #0 {
+; NOFMA-LABEL: f11:
+; NOFMA: # %bb.0: # %entry
+; NOFMA-NEXT:mulss %xmm1, %xmm0
+; NOFMA-NEXT:addss %xmm2, %xmm0
+; NOFMA-NEXT:retq
+;
+; FMA-LABEL: f11:
+; FMA: # %bb.0: # %entry
+; FMA-NEXT:vmulss %xmm1, %xmm0, %xmm0
+; FMA-NEXT:vaddss %xmm2, %xmm0, %xmm0
+; FMA-NEXT:retq
+;
+; FMA4-LABEL: f11:
+; FMA4: # %bb.0: # %entry
+; FMA4-NEXT:vmulss %xmm1, %xmm0, %xmm0
+; FMA4-NEXT:vaddss %xmm2, %xmm0, %xmm0
+; FMA4-NEXT:retq
+entry:
+ %3 = call float @llvm.experimental.constrained.fmul.f32(float %0, float %1,
+ metadata !"round.dynamic",
+ metadata !"fpexcept.strict") #0
+ %4 = call float @llvm.experimental.constrained.fadd.f32(float %3, float %2,
+ metadata !"round.dynamic",
+ metadata !"fpexcept.strict") #0
+ ret float %4
+}
+
+; Verify constrained fmul and fadd aren't fused.
+define double @f12(double %0, double %1, double %2) #0 {
+; NOFMA-LABEL: f12:
+; NOFMA: # %bb.0: # %entry
+; NOFMA-NEXT:mulsd %xmm1, %xmm0
+; NOFMA-NEXT:addsd %xmm2, %xmm0
+; NOFMA-NEXT:retq
+;
+; FMA-LABEL: f12:
+; FMA: # %bb.0: # %entry
+; FMA-NEXT:vmulsd %xmm1, %xmm0, %xmm0
+; FMA-NEXT:vaddsd %xmm2, %xmm0, %xmm0
+; FMA-NEXT:retq
+;
+; FMA4-LABEL: f12:
+; FMA4: # %bb.0: # %entry
+; FMA4-NEXT:vmulsd %xmm1, %xmm0, %xmm0
+; FMA4-NEXT:vaddsd %xmm2, %xmm0, %xmm0
+; FMA4-NEXT:retq
+entry:
+ %3 = call double @llvm.experimental.constrained.fmul.f64(double %0, double %1,
+ metadata !"round.dynamic",
+ metadata !"fpexcept.strict") #0
+ %4 = call double @llvm.experimental.constrained.fadd.f64(double %3, double %2,
+ metadata !"round.dynamic",
+ metadata !"fpexcept.strict") #0
+ ret double %4
+}
+
+; Verify that fmuladd(3.5) isn't simplified when the rounding mode is
+; unknown.
+define float @f15() #0 {
+; NOFMA-LABEL: f15:
+; NOFMA: # %bb.0: # %entry
+; NOFMA-NEXT:movss {{.*#+}} xmm1 = mem[0],zero,zero,zero
+; NOFMA-NEXT:movaps %xmm1, %xmm0
+; NOFMA-NEXT:mulss %xmm1, %xmm0
+; NOFMA-NEXT:addss %xmm1, %xmm0
+; NOFMA-NEXT:retq
+;
+; FMA-LABEL: f15:
+; FMA: # %bb.0: # %entry
+; FMA-NEXT:vmovss {{.*#+}} xmm0 = mem[0],zero,zero,zero
+; FMA-NEXT:vfmadd213ss {{.*#+}} xmm0 = (xmm0 * xmm0) + xmm0
+; FMA-NEXT:retq
+;
+; FMA4-LABEL: f15:
+; FMA4: # %bb.0: # %entry
+; FMA4-NEXT:vmovss {{.*#+}} xmm0 = mem[0],zero,zero,zero
+; FMA4-NEXT:vfmaddss %xmm0, %xmm0, %xmm0, %xmm0
+; FMA4-NEXT:retq
+entry:
+ %result = call float @llvm.experimental.constrained.fmuladd.f32(
+ float 3.5,
+ float 3.5,
+ float 3.5,
+ metadata !"round.dynamic",
+ metadata !"fpexcept.strict") #0
+ ret float %result
+}
+
+; Verify that fmuladd(42.1) isn't simplified when the rounding mode is
+; unknown.
+define double @f16() #0 {
+; NOFMA-LABEL: f16:
+; NOFMA: # %bb.0: # %entry
+; NOFMA-NEXT:movsd {{.*#+}} xmm1 = mem[0],zero
+; NOFMA-NEXT:movapd %xmm1, %xmm0
+; NOFMA-NEXT:mulsd %xmm1, %xmm0
+; NOFMA-NEXT:addsd %xmm1, %xmm0
+; NOFMA-NEXT:retq
+;
+; FMA-LABEL: f16:
+; FMA: # %bb.0: # %entry
+; FMA-NEXT:vmovsd {{.*#+}} xmm0 = mem[0],zero
+; FMA-NEXT:vfmadd213sd {{.*#+}} xmm0 = (xmm0 * xmm0) + xmm0
+; FMA-NEXT:
[PATCH] D72820: [FPEnv] Add pragma FP_CONTRACT support under strict FP.
craig.topper accepted this revision. craig.topper added a comment. This revision is now accepted and ready to land. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72820/new/ https://reviews.llvm.org/D72820 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D72820: [FPEnv] Add pragma FP_CONTRACT support under strict FP.
pengfei marked an inline comment as done. pengfei added inline comments. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3381 + "constrained mode"); +FMulAdd = Builder.CreateCall( +CGF.CGM.getIntrinsic(llvm::Intrinsic::experimental_constrained_fmuladd, kpn wrote: > craig.topper wrote: > > kpn wrote: > > > craig.topper wrote: > > > > Doesn't this need to be CreateConstrainedFPCall so that the strictfp > > > > attribute is added? That will take care of adding the metadata operands > > > > too. > > > Is this code tested? I ran into a bug yesterday where CreateCall was used > > > with a constrained intrinsic and the Instruction class blew up because > > > the function signature was wrong. I wasn't passing in the metadata > > > arguments. > > > > > > So, yes, it should be, and it would might make sense for the patch to > > > have test coverage that catches any other cases of this. > > This code is copying the metadata arguments from the fmul intrinsic, MulOp. > > that’s the getOperand(2) and getOperand(3). > Ah, yes, thanks. Your comment about the attribute is still valid, though. > And, yes, using CreateConstrainedFPCall() is the easiest way to fix the > attribute. Yes, it's the best choice. Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72820/new/ https://reviews.llvm.org/D72820 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D72820: [FPEnv] Add pragma FP_CONTRACT support under strict FP.
pengfei updated this revision to Diff 240474.
pengfei added a comment.
Address review comment.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D72820/new/
https://reviews.llvm.org/D72820
Files:
clang/lib/CodeGen/CGExprScalar.cpp
clang/test/CodeGen/constrained-math-builtins.c
llvm/docs/LangRef.rst
llvm/include/llvm/CodeGen/BasicTTIImpl.h
llvm/include/llvm/IR/ConstrainedOps.def
llvm/include/llvm/IR/Intrinsics.td
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
llvm/test/CodeGen/X86/fp-intrinsics-fma.ll
llvm/test/TableGen/GlobalISelEmitter-input-discard.td
Index: llvm/test/TableGen/GlobalISelEmitter-input-discard.td
===
--- llvm/test/TableGen/GlobalISelEmitter-input-discard.td
+++ llvm/test/TableGen/GlobalISelEmitter-input-discard.td
@@ -15,7 +15,7 @@
// GISEL-NEXT: GIM_CheckType, /*MI*/0, /*Op*/2, /*Type*/GILLT_s32,
// GISEL-NEXT: GIM_CheckType, /*MI*/0, /*Op*/3, /*Type*/GILLT_s32,
// GISEL-NEXT: GIM_CheckRegBankForClass, /*MI*/0, /*Op*/0, /*RC*/MyTarget::GPR32RegClassID,
-// GISEL-NEXT: // (intrinsic_w_chain:{ *:[i32] } 248:{ *:[iPTR] }, srcvalue:{ *:[i32] }, i32:{ *:[i32] }:$src1) => (FOO:{ *:[i32] } (IMPLICIT_DEF:{ *:[i32] }), GPR32:{ *:[i32] }:$src1)
+// GISEL-NEXT: // (intrinsic_w_chain:{ *:[i32] } 249:{ *:[iPTR] }, srcvalue:{ *:[i32] }, i32:{ *:[i32] }:$src1) => (FOO:{ *:[i32] } (IMPLICIT_DEF:{ *:[i32] }), GPR32:{ *:[i32] }:$src1)
// GISEL-NEXT: GIR_MakeTempReg, /*TempRegID*/0, /*TypeID*/GILLT_s32,
// GISEL-NEXT: GIR_BuildMI, /*InsnID*/1, /*Opcode*/TargetOpcode::IMPLICIT_DEF,
// GISEL-NEXT: GIR_AddTempRegister, /*InsnID*/1, /*TempRegID*/0, /*TempRegFlags*/RegState::Define,
Index: llvm/test/CodeGen/X86/fp-intrinsics-fma.ll
===
--- llvm/test/CodeGen/X86/fp-intrinsics-fma.ll
+++ llvm/test/CodeGen/X86/fp-intrinsics-fma.ll
@@ -3,6 +3,104 @@
; RUN: llc -O3 -mtriple=x86_64-pc-linux -mattr=+fma < %s | FileCheck %s --check-prefixes=COMMON,FMA
; RUN: llc -O3 -mtriple=x86_64-pc-linux -mattr=+avx512f < %s | FileCheck %s --check-prefixes=COMMON,FMA
+; Verify constrained fmul and fadd aren't fused.
+define float @f11(float %0, float %1, float %2) #0 {
+; NOFMA-LABEL: f11:
+; NOFMA: # %bb.0: # %entry
+; NOFMA-NEXT:mulss %xmm1, %xmm0
+; NOFMA-NEXT:addss %xmm2, %xmm0
+; NOFMA-NEXT:retq
+;
+; FMA-LABEL: f11:
+; FMA: # %bb.0: # %entry
+; FMA-NEXT:vmulss %xmm1, %xmm0, %xmm0
+; FMA-NEXT:vaddss %xmm2, %xmm0, %xmm0
+; FMA-NEXT:retq
+entry:
+ %3 = call float @llvm.experimental.constrained.fmul.f32(float %0, float %1,
+ metadata !"round.dynamic",
+ metadata !"fpexcept.strict") #0
+ %4 = call float @llvm.experimental.constrained.fadd.f32(float %3, float %2,
+ metadata !"round.dynamic",
+ metadata !"fpexcept.strict") #0
+ ret float %4
+}
+
+; Verify constrained fmul and fadd aren't fused.
+define double @f12(double %0, double %1, double %2) #0 {
+; NOFMA-LABEL: f12:
+; NOFMA: # %bb.0: # %entry
+; NOFMA-NEXT:mulsd %xmm1, %xmm0
+; NOFMA-NEXT:addsd %xmm2, %xmm0
+; NOFMA-NEXT:retq
+;
+; FMA-LABEL: f12:
+; FMA: # %bb.0: # %entry
+; FMA-NEXT:vmulsd %xmm1, %xmm0, %xmm0
+; FMA-NEXT:vaddsd %xmm2, %xmm0, %xmm0
+; FMA-NEXT:retq
+entry:
+ %3 = call double @llvm.experimental.constrained.fmul.f64(double %0, double %1,
+ metadata !"round.dynamic",
+ metadata !"fpexcept.strict") #0
+ %4 = call double @llvm.experimental.constrained.fadd.f64(double %3, double %2,
+ metadata !"round.dynamic",
+ metadata !"fpexcept.strict") #0
+ ret double %4
+}
+
+; Verify that fmuladd(3.5) isn't simplified when the rounding mode is
+; unknown.
+define float @f15() #0 {
+; NOFMA-LABEL: f15:
+; NOFMA: # %bb.0: # %entry
+; NOFMA-NEXT:movss {{.*#+}} xmm1 = mem[0],zero,zero,zero
+; NOFMA-NEXT:movaps %xmm1, %xmm0
+; NOFMA-NEXT:mulss %xmm1, %xmm0
+; NOFMA-NEXT:addss %xmm1, %xmm0
+; NOFMA-NEXT:retq
+;
+; FMA-LABEL: f15:
+; FMA: # %bb.0: # %entry
+; FMA-NEXT:vmovss {{.*#+}} xmm0 = mem[0],zero,zero,zero
+; FMA-NEXT:vfmadd213ss {{.*#+}} xmm0 = (xmm0 * xmm0) + xmm0
+; FMA-NEXT:retq
+entry:
+ %result = call float @llvm.experimental.constrained.fmuladd.f32(
+ float 3.5,
+ float 3.5,
+ float 3.5,
+ metadata
[PATCH] D72820: [FPEnv] Add pragma FP_CONTRACT support under strict FP.
kpn added inline comments. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3381 + "constrained mode"); +FMulAdd = Builder.CreateCall( +CGF.CGM.getIntrinsic(llvm::Intrinsic::experimental_constrained_fmuladd, craig.topper wrote: > kpn wrote: > > craig.topper wrote: > > > Doesn't this need to be CreateConstrainedFPCall so that the strictfp > > > attribute is added? That will take care of adding the metadata operands > > > too. > > Is this code tested? I ran into a bug yesterday where CreateCall was used > > with a constrained intrinsic and the Instruction class blew up because the > > function signature was wrong. I wasn't passing in the metadata arguments. > > > > So, yes, it should be, and it would might make sense for the patch to have > > test coverage that catches any other cases of this. > This code is copying the metadata arguments from the fmul intrinsic, MulOp. > that’s the getOperand(2) and getOperand(3). Ah, yes, thanks. Your comment about the attribute is still valid, though. And, yes, using CreateConstrainedFPCall() is the easiest way to fix the attribute. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72820/new/ https://reviews.llvm.org/D72820 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D72820: [FPEnv] Add pragma FP_CONTRACT support under strict FP.
craig.topper added inline comments. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3381 + "constrained mode"); +FMulAdd = Builder.CreateCall( +CGF.CGM.getIntrinsic(llvm::Intrinsic::experimental_constrained_fmuladd, kpn wrote: > craig.topper wrote: > > Doesn't this need to be CreateConstrainedFPCall so that the strictfp > > attribute is added? That will take care of adding the metadata operands too. > Is this code tested? I ran into a bug yesterday where CreateCall was used > with a constrained intrinsic and the Instruction class blew up because the > function signature was wrong. I wasn't passing in the metadata arguments. > > So, yes, it should be, and it would might make sense for the patch to have > test coverage that catches any other cases of this. This code is copying the metadata arguments from the fmul intrinsic, MulOp. that’s the getOperand(2) and getOperand(3). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72820/new/ https://reviews.llvm.org/D72820 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D72820: [FPEnv] Add pragma FP_CONTRACT support under strict FP.
kpn added inline comments. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3381 + "constrained mode"); +FMulAdd = Builder.CreateCall( +CGF.CGM.getIntrinsic(llvm::Intrinsic::experimental_constrained_fmuladd, craig.topper wrote: > Doesn't this need to be CreateConstrainedFPCall so that the strictfp > attribute is added? That will take care of adding the metadata operands too. Is this code tested? I ran into a bug yesterday where CreateCall was used with a constrained intrinsic and the Instruction class blew up because the function signature was wrong. I wasn't passing in the metadata arguments. So, yes, it should be, and it would might make sense for the patch to have test coverage that catches any other cases of this. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72820/new/ https://reviews.llvm.org/D72820 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D72820: [FPEnv] Add pragma FP_CONTRACT support under strict FP.
craig.topper added inline comments. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3381 + "constrained mode"); +FMulAdd = Builder.CreateCall( +CGF.CGM.getIntrinsic(llvm::Intrinsic::experimental_constrained_fmuladd, Doesn't this need to be CreateConstrainedFPCall so that the strictfp attribute is added? That will take care of adding the metadata operands too. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72820/new/ https://reviews.llvm.org/D72820 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D72820: [FPEnv] Add pragma FP_CONTRACT support under strict FP.
rjmccall added a comment. Remember that the design is that constrained intrinsics must be used whenever *any* code in the function is constrained. It is not unreasonable that part of the function might be constrained and the rest subject to fast-math; it'd be a shame if the intrinsics couldn't even express that. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72820/new/ https://reviews.llvm.org/D72820 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D72820: [FPEnv] Add pragma FP_CONTRACT support under strict FP.
andrew.w.kaylor added inline comments. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3436 +} + } + cameron.mcinally wrote: > andrew.w.kaylor wrote: > > cameron.mcinally wrote: > > > cameron.mcinally wrote: > > > > cameron.mcinally wrote: > > > > > I don't think it's safe to fuse a FMUL and FADD if the intermediate > > > > > rounding isn't exactly the same as those individual operations. > > > > > FMULADD doesn't guarantee that, does it? > > > > To be clear, we could miss very-edge-case overflow/underflow exceptions. > > > Ah, but I see C/C++ FP_CONTRACT allows the exceptions to be optimized > > > away. Sorry for the noise. > > We've talked about this before but I don't think we ever documented a > > decision as to whether we want to allow constrained intrinsics and fast > > math flags to be combined. This patch moves that decision into clang's > > decision to generate this intrinsic or not. > > > > I think it definitely makes sense in the case of fp contraction, because > > even if a user cares about value safety they might want FMA, which is > > theorectically more accurate than the separate values even though it > > produces a different value. This is consistent with gcc (which produces FMA > > under "-ffp-contract=fast -fno-fast-math") and icc (which produced FMA > > under "-fp-model strict -fma"). > > > > For the record, I also think it makes sense to use nnan, ninf, and nsz with > > constrained intrinsics. > You had me until: > > >For the record, I also think it makes sense to use nnan, ninf, and nsz with > >constrained intrinsics. > > To be clear, we'd need them for the `fast` case, but I don't see a lot of > value for the `strict` case. > > We definitely want reassoc/recip/etc for the `optimized but trap-safe` case, > so that's enough to require FMF flags on constrained intrinsics alone. > > We should probably break this conversation out into an llvm-dev thread... I agree about starting an llvm-dev thread. I'll send something out unless you've already done so by the time I finish typing it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72820/new/ https://reviews.llvm.org/D72820 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D72820: [FPEnv] Add pragma FP_CONTRACT support under strict FP.
cameron.mcinally added inline comments. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3436 +} + } + andrew.w.kaylor wrote: > cameron.mcinally wrote: > > cameron.mcinally wrote: > > > cameron.mcinally wrote: > > > > I don't think it's safe to fuse a FMUL and FADD if the intermediate > > > > rounding isn't exactly the same as those individual operations. FMULADD > > > > doesn't guarantee that, does it? > > > To be clear, we could miss very-edge-case overflow/underflow exceptions. > > Ah, but I see C/C++ FP_CONTRACT allows the exceptions to be optimized away. > > Sorry for the noise. > We've talked about this before but I don't think we ever documented a > decision as to whether we want to allow constrained intrinsics and fast math > flags to be combined. This patch moves that decision into clang's decision to > generate this intrinsic or not. > > I think it definitely makes sense in the case of fp contraction, because even > if a user cares about value safety they might want FMA, which is > theorectically more accurate than the separate values even though it produces > a different value. This is consistent with gcc (which produces FMA under > "-ffp-contract=fast -fno-fast-math") and icc (which produced FMA under > "-fp-model strict -fma"). > > For the record, I also think it makes sense to use nnan, ninf, and nsz with > constrained intrinsics. You had me until: >For the record, I also think it makes sense to use nnan, ninf, and nsz with >constrained intrinsics. To be clear, we'd need them for the `fast` case, but I don't see a lot of value for the `strict` case. We definitely want reassoc/recip/etc for the `optimized but trap-safe` case, so that's enough to require FMF flags on constrained intrinsics alone. We should probably break this conversation out into an llvm-dev thread... Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72820/new/ https://reviews.llvm.org/D72820 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D72820: [FPEnv] Add pragma FP_CONTRACT support under strict FP.
pengfei marked an inline comment as done.
pengfei added inline comments.
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3381
+ Addend->getType()),
+{MulOp0, MulOp1, Addend, MulOp->getOperand(2), MulOp->getOperand(3)});
+ else
andrew.w.kaylor wrote:
> You shouldn't just assume that MulOp is a constrained intrinsic. Cast to
> ConstrainedFPIntrinsic and use ConstrainedFPIntrinsic::getRoundingMode() and
> ConstrainedFPIntrinsic::getExceptionBehavior(). The cast will effectively
> assert that MulOp is a constrained intrisic. I think that should always be
> true.
I prefer to reuse the operands from the fmul intrinsic here. 1). fmuladd always
has the same exception/rounding mode with fmul. 2). the function
getRoundingMode/getExceptionBehavior just return a enum value. We need more
code to turn them into Value type.
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3423
+ if (Builder.getIsFPConstrained()) {
+if (auto *LHSBinOp = dyn_cast(op.LHS)) {
andrew.w.kaylor wrote:
> I don't think we should ever non-constrained create FMul instructions if
> Builder is in FP constrained mode, but you should assert that somewhere
> above. Maybe move this block above line 3409 and add:
>
> assert(LHSBinOp->getOpcode() != llvm::Instruction::FMul &&
> RHSBinOp->getOpcode() != llvm::Instruction::FMul);
Add assertion in line 3380. We only need to check once there.
Comment at: llvm/test/TableGen/GlobalISelEmitter-input-discard.td:18
// GISEL-NEXT: GIM_CheckRegBankForClass, /*MI*/0, /*Op*/0,
/*RC*/MyTarget::GPR32RegClassID,
-// GISEL-NEXT: // (intrinsic_w_chain:{ *:[i32] } 248:{ *:[iPTR] }, srcvalue:{
*:[i32] }, i32:{ *:[i32] }:$src1) => (FOO:{ *:[i32] } (IMPLICIT_DEF:{ *:[i32]
}), GPR32:{ *:[i32] }:$src1)
+// GISEL-NEXT: // (intrinsic_w_chain:{ *:[i32] } 249:{ *:[iPTR] }, srcvalue:{
*:[i32] }, i32:{ *:[i32] }:$src1) => (FOO:{ *:[i32] } (IMPLICIT_DEF:{ *:[i32]
}), GPR32:{ *:[i32] }:$src1)
// GISEL-NEXT: GIR_MakeTempReg, /*TempRegID*/0, /*TypeID*/GILLT_s32,
It's strange the number is affected. I haven't found any cause.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D72820/new/
https://reviews.llvm.org/D72820
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D72820: [FPEnv] Add pragma FP_CONTRACT support under strict FP.
pengfei updated this revision to Diff 238770.
pengfei added a comment.
Remove unnecessary comment.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D72820/new/
https://reviews.llvm.org/D72820
Files:
clang/lib/CodeGen/CGExprScalar.cpp
clang/test/CodeGen/constrained-math-builtins.c
llvm/docs/LangRef.rst
llvm/include/llvm/CodeGen/BasicTTIImpl.h
llvm/include/llvm/IR/ConstrainedOps.def
llvm/include/llvm/IR/Intrinsics.td
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
llvm/test/CodeGen/X86/fp-intrinsics-fma.ll
llvm/test/TableGen/GlobalISelEmitter-input-discard.td
Index: llvm/test/TableGen/GlobalISelEmitter-input-discard.td
===
--- llvm/test/TableGen/GlobalISelEmitter-input-discard.td
+++ llvm/test/TableGen/GlobalISelEmitter-input-discard.td
@@ -15,7 +15,7 @@
// GISEL-NEXT: GIM_CheckType, /*MI*/0, /*Op*/2, /*Type*/GILLT_s32,
// GISEL-NEXT: GIM_CheckType, /*MI*/0, /*Op*/3, /*Type*/GILLT_s32,
// GISEL-NEXT: GIM_CheckRegBankForClass, /*MI*/0, /*Op*/0, /*RC*/MyTarget::GPR32RegClassID,
-// GISEL-NEXT: // (intrinsic_w_chain:{ *:[i32] } 248:{ *:[iPTR] }, srcvalue:{ *:[i32] }, i32:{ *:[i32] }:$src1) => (FOO:{ *:[i32] } (IMPLICIT_DEF:{ *:[i32] }), GPR32:{ *:[i32] }:$src1)
+// GISEL-NEXT: // (intrinsic_w_chain:{ *:[i32] } 249:{ *:[iPTR] }, srcvalue:{ *:[i32] }, i32:{ *:[i32] }:$src1) => (FOO:{ *:[i32] } (IMPLICIT_DEF:{ *:[i32] }), GPR32:{ *:[i32] }:$src1)
// GISEL-NEXT: GIR_MakeTempReg, /*TempRegID*/0, /*TypeID*/GILLT_s32,
// GISEL-NEXT: GIR_BuildMI, /*InsnID*/1, /*Opcode*/TargetOpcode::IMPLICIT_DEF,
// GISEL-NEXT: GIR_AddTempRegister, /*InsnID*/1, /*TempRegID*/0, /*TempRegFlags*/RegState::Define,
Index: llvm/test/CodeGen/X86/fp-intrinsics-fma.ll
===
--- llvm/test/CodeGen/X86/fp-intrinsics-fma.ll
+++ llvm/test/CodeGen/X86/fp-intrinsics-fma.ll
@@ -3,6 +3,104 @@
; RUN: llc -O3 -mtriple=x86_64-pc-linux -mattr=+fma < %s | FileCheck %s --check-prefixes=COMMON,FMA
; RUN: llc -O3 -mtriple=x86_64-pc-linux -mattr=+avx512f < %s | FileCheck %s --check-prefixes=COMMON,FMA
+; Verify constrained fmul and fadd aren't fused.
+define float @f11(float %0, float %1, float %2) #0 {
+; NOFMA-LABEL: f11:
+; NOFMA: # %bb.0: # %entry
+; NOFMA-NEXT:mulss %xmm1, %xmm0
+; NOFMA-NEXT:addss %xmm2, %xmm0
+; NOFMA-NEXT:retq
+;
+; FMA-LABEL: f11:
+; FMA: # %bb.0: # %entry
+; FMA-NEXT:vmulss %xmm1, %xmm0, %xmm0
+; FMA-NEXT:vaddss %xmm2, %xmm0, %xmm0
+; FMA-NEXT:retq
+entry:
+ %3 = call float @llvm.experimental.constrained.fmul.f32(float %0, float %1,
+ metadata !"round.dynamic",
+ metadata !"fpexcept.strict") #0
+ %4 = call float @llvm.experimental.constrained.fadd.f32(float %3, float %2,
+ metadata !"round.dynamic",
+ metadata !"fpexcept.strict") #0
+ ret float %4
+}
+
+; Verify constrained fmul and fadd aren't fused.
+define double @f12(double %0, double %1, double %2) #0 {
+; NOFMA-LABEL: f12:
+; NOFMA: # %bb.0: # %entry
+; NOFMA-NEXT:mulsd %xmm1, %xmm0
+; NOFMA-NEXT:addsd %xmm2, %xmm0
+; NOFMA-NEXT:retq
+;
+; FMA-LABEL: f12:
+; FMA: # %bb.0: # %entry
+; FMA-NEXT:vmulsd %xmm1, %xmm0, %xmm0
+; FMA-NEXT:vaddsd %xmm2, %xmm0, %xmm0
+; FMA-NEXT:retq
+entry:
+ %3 = call double @llvm.experimental.constrained.fmul.f64(double %0, double %1,
+ metadata !"round.dynamic",
+ metadata !"fpexcept.strict") #0
+ %4 = call double @llvm.experimental.constrained.fadd.f64(double %3, double %2,
+ metadata !"round.dynamic",
+ metadata !"fpexcept.strict") #0
+ ret double %4
+}
+
+; Verify that fmuladd(3.5) isn't simplified when the rounding mode is
+; unknown.
+define float @f15() #0 {
+; NOFMA-LABEL: f15:
+; NOFMA: # %bb.0: # %entry
+; NOFMA-NEXT:movss {{.*#+}} xmm1 = mem[0],zero,zero,zero
+; NOFMA-NEXT:movaps %xmm1, %xmm0
+; NOFMA-NEXT:mulss %xmm1, %xmm0
+; NOFMA-NEXT:addss %xmm1, %xmm0
+; NOFMA-NEXT:retq
+;
+; FMA-LABEL: f15:
+; FMA: # %bb.0: # %entry
+; FMA-NEXT:vmovss {{.*#+}} xmm0 = mem[0],zero,zero,zero
+; FMA-NEXT:vfmadd213ss {{.*#+}} xmm0 = (xmm0 * xmm0) + xmm0
+; FMA-NEXT:retq
+entry:
+ %result = call float @llvm.experimental.constrained.fmuladd.f32(
+ float 3.5,
+ float 3.5,
+ float 3.5,
+ metadata
[PATCH] D72820: [FPEnv] Add pragma FP_CONTRACT support under strict FP.
pengfei updated this revision to Diff 238769.
pengfei marked 7 inline comments as done.
pengfei added a comment.
Address review comments.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D72820/new/
https://reviews.llvm.org/D72820
Files:
clang/lib/CodeGen/CGExprScalar.cpp
clang/test/CodeGen/constrained-math-builtins.c
llvm/docs/LangRef.rst
llvm/include/llvm/CodeGen/BasicTTIImpl.h
llvm/include/llvm/IR/ConstrainedOps.def
llvm/include/llvm/IR/Intrinsics.td
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
llvm/test/CodeGen/X86/fp-intrinsics-fma.ll
llvm/test/TableGen/GlobalISelEmitter-input-discard.td
Index: llvm/test/TableGen/GlobalISelEmitter-input-discard.td
===
--- llvm/test/TableGen/GlobalISelEmitter-input-discard.td
+++ llvm/test/TableGen/GlobalISelEmitter-input-discard.td
@@ -15,7 +15,7 @@
// GISEL-NEXT: GIM_CheckType, /*MI*/0, /*Op*/2, /*Type*/GILLT_s32,
// GISEL-NEXT: GIM_CheckType, /*MI*/0, /*Op*/3, /*Type*/GILLT_s32,
// GISEL-NEXT: GIM_CheckRegBankForClass, /*MI*/0, /*Op*/0, /*RC*/MyTarget::GPR32RegClassID,
-// GISEL-NEXT: // (intrinsic_w_chain:{ *:[i32] } 248:{ *:[iPTR] }, srcvalue:{ *:[i32] }, i32:{ *:[i32] }:$src1) => (FOO:{ *:[i32] } (IMPLICIT_DEF:{ *:[i32] }), GPR32:{ *:[i32] }:$src1)
+// GISEL-NEXT: // (intrinsic_w_chain:{ *:[i32] } 249:{ *:[iPTR] }, srcvalue:{ *:[i32] }, i32:{ *:[i32] }:$src1) => (FOO:{ *:[i32] } (IMPLICIT_DEF:{ *:[i32] }), GPR32:{ *:[i32] }:$src1)
// GISEL-NEXT: GIR_MakeTempReg, /*TempRegID*/0, /*TypeID*/GILLT_s32,
// GISEL-NEXT: GIR_BuildMI, /*InsnID*/1, /*Opcode*/TargetOpcode::IMPLICIT_DEF,
// GISEL-NEXT: GIR_AddTempRegister, /*InsnID*/1, /*TempRegID*/0, /*TempRegFlags*/RegState::Define,
Index: llvm/test/CodeGen/X86/fp-intrinsics-fma.ll
===
--- llvm/test/CodeGen/X86/fp-intrinsics-fma.ll
+++ llvm/test/CodeGen/X86/fp-intrinsics-fma.ll
@@ -3,6 +3,104 @@
; RUN: llc -O3 -mtriple=x86_64-pc-linux -mattr=+fma < %s | FileCheck %s --check-prefixes=COMMON,FMA
; RUN: llc -O3 -mtriple=x86_64-pc-linux -mattr=+avx512f < %s | FileCheck %s --check-prefixes=COMMON,FMA
+; Verify constrained fmul and fadd aren't fused.
+define float @f11(float %0, float %1, float %2) #0 {
+; NOFMA-LABEL: f11:
+; NOFMA: # %bb.0: # %entry
+; NOFMA-NEXT:mulss %xmm1, %xmm0
+; NOFMA-NEXT:addss %xmm2, %xmm0
+; NOFMA-NEXT:retq
+;
+; FMA-LABEL: f11:
+; FMA: # %bb.0: # %entry
+; FMA-NEXT:vmulss %xmm1, %xmm0, %xmm0
+; FMA-NEXT:vaddss %xmm2, %xmm0, %xmm0
+; FMA-NEXT:retq
+entry:
+ %3 = call float @llvm.experimental.constrained.fmul.f32(float %0, float %1,
+ metadata !"round.dynamic",
+ metadata !"fpexcept.strict") #0
+ %4 = call float @llvm.experimental.constrained.fadd.f32(float %3, float %2,
+ metadata !"round.dynamic",
+ metadata !"fpexcept.strict") #0
+ ret float %4
+}
+
+; Verify constrained fmul and fadd aren't fused.
+define double @f12(double %0, double %1, double %2) #0 {
+; NOFMA-LABEL: f12:
+; NOFMA: # %bb.0: # %entry
+; NOFMA-NEXT:mulsd %xmm1, %xmm0
+; NOFMA-NEXT:addsd %xmm2, %xmm0
+; NOFMA-NEXT:retq
+;
+; FMA-LABEL: f12:
+; FMA: # %bb.0: # %entry
+; FMA-NEXT:vmulsd %xmm1, %xmm0, %xmm0
+; FMA-NEXT:vaddsd %xmm2, %xmm0, %xmm0
+; FMA-NEXT:retq
+entry:
+ %3 = call double @llvm.experimental.constrained.fmul.f64(double %0, double %1,
+ metadata !"round.dynamic",
+ metadata !"fpexcept.strict") #0
+ %4 = call double @llvm.experimental.constrained.fadd.f64(double %3, double %2,
+ metadata !"round.dynamic",
+ metadata !"fpexcept.strict") #0
+ ret double %4
+}
+
+; Verify that fmuladd(3.5) isn't simplified when the rounding mode is
+; unknown.
+define float @f15() #0 {
+; NOFMA-LABEL: f15:
+; NOFMA: # %bb.0: # %entry
+; NOFMA-NEXT:movss {{.*#+}} xmm1 = mem[0],zero,zero,zero
+; NOFMA-NEXT:movaps %xmm1, %xmm0
+; NOFMA-NEXT:mulss %xmm1, %xmm0
+; NOFMA-NEXT:addss %xmm1, %xmm0
+; NOFMA-NEXT:retq
+;
+; FMA-LABEL: f15:
+; FMA: # %bb.0: # %entry
+; FMA-NEXT:vmovss {{.*#+}} xmm0 = mem[0],zero,zero,zero
+; FMA-NEXT:vfmadd213ss {{.*#+}} xmm0 = (xmm0 * xmm0) + xmm0
+; FMA-NEXT:retq
+entry:
+ %result = call float @llvm.experimental.constrained.fmuladd.f32(
+ float 3.5,
+ float 3.5,
+ float 3.5,
+
[PATCH] D72820: [FPEnv] Add pragma FP_CONTRACT support under strict FP.
andrew.w.kaylor added inline comments. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3436 +} + } + cameron.mcinally wrote: > cameron.mcinally wrote: > > cameron.mcinally wrote: > > > I don't think it's safe to fuse a FMUL and FADD if the intermediate > > > rounding isn't exactly the same as those individual operations. FMULADD > > > doesn't guarantee that, does it? > > To be clear, we could miss very-edge-case overflow/underflow exceptions. > Ah, but I see C/C++ FP_CONTRACT allows the exceptions to be optimized away. > Sorry for the noise. We've talked about this before but I don't think we ever documented a decision as to whether we want to allow constrained intrinsics and fast math flags to be combined. This patch moves that decision into clang's decision to generate this intrinsic or not. I think it definitely makes sense in the case of fp contraction, because even if a user cares about value safety they might want FMA, which is theorectically more accurate than the separate values even though it produces a different value. This is consistent with gcc (which produces FMA under "-ffp-contract=fast -fno-fast-math") and icc (which produced FMA under "-fp-model strict -fma"). For the record, I also think it makes sense to use nnan, ninf, and nsz with constrained intrinsics. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72820/new/ https://reviews.llvm.org/D72820 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D72820: [FPEnv] Add pragma FP_CONTRACT support under strict FP.
cameron.mcinally added inline comments. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3436 +} + } + cameron.mcinally wrote: > cameron.mcinally wrote: > > I don't think it's safe to fuse a FMUL and FADD if the intermediate > > rounding isn't exactly the same as those individual operations. FMULADD > > doesn't guarantee that, does it? > To be clear, we could miss very-edge-case overflow/underflow exceptions. Ah, but I see C/C++ FP_CONTRACT allows the exceptions to be optimized away. Sorry for the noise. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72820/new/ https://reviews.llvm.org/D72820 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D72820: [FPEnv] Add pragma FP_CONTRACT support under strict FP.
cameron.mcinally added inline comments. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3436 +} + } + cameron.mcinally wrote: > I don't think it's safe to fuse a FMUL and FADD if the intermediate rounding > isn't exactly the same as those individual operations. FMULADD doesn't > guarantee that, does it? To be clear, we could miss very-edge-case overflow/underflow exceptions. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72820/new/ https://reviews.llvm.org/D72820 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D72820: [FPEnv] Add pragma FP_CONTRACT support under strict FP.
cameron.mcinally added inline comments. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3436 +} + } + I don't think it's safe to fuse a FMUL and FADD if the intermediate rounding isn't exactly the same as those individual operations. FMULADD doesn't guarantee that, does it? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72820/new/ https://reviews.llvm.org/D72820 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D72820: [FPEnv] Add pragma FP_CONTRACT support under strict FP.
andrew.w.kaylor added inline comments.
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3381
+ Addend->getType()),
+{MulOp0, MulOp1, Addend, MulOp->getOperand(2), MulOp->getOperand(3)});
+ else
You shouldn't just assume that MulOp is a constrained intrinsic. Cast to
ConstrainedFPIntrinsic and use ConstrainedFPIntrinsic::getRoundingMode() and
ConstrainedFPIntrinsic::getExceptionBehavior(). The cast will effectively
assert that MulOp is a constrained intrisic. I think that should always be true.
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3423
+ if (Builder.getIsFPConstrained()) {
+if (auto *LHSBinOp = dyn_cast(op.LHS)) {
I don't think we should ever non-constrained create FMul instructions if
Builder is in FP constrained mode, but you should assert that somewhere above.
Maybe move this block above line 3409 and add:
assert(LHSBinOp->getOpcode() != llvm::Instruction::FMul &&
RHSBinOp->getOpcode() != llvm::Instruction::FMul);
Comment at: clang/test/CodeGen/constrained-math-builtins.c:160
+// CHECK: declare x86_fp80
@llvm.experimental.constrained.fmuladd.f80(x86_fp80, x86_fp80, x86_fp80,
metadata, metadata)
+};
I'd like to see a test that verifies the calls generated in the function and
specifically a test that verifies that the constrained fneg is generated if
needed.
Comment at: llvm/docs/LangRef.rst:16094
+
+The fourth and fifth arguments specifie the exception behavior as described
+above.
s/specifie/specify
s/the exception behavior/the rounding mode and exception behavior
Comment at: llvm/docs/LangRef.rst:16104
+
+ %0 = call float @llvm.experimental.constrained.fmuladd.f32(%a, %b, %c)
+
missing metadata arguments
Comment at: llvm/include/llvm/CodeGen/BasicTTIImpl.h:1515
ConcreteTTI->getArithmeticInstrCost(BinaryOperator::FAdd, RetTy);
+// FIXME: Is constrained intrinsic' cost equal to it's no strict one?
+if (IID == Intrinsic::experimental_constrained_fmuladd)
I don't think that matters. The cost calculation here is a conservative
estimate based on the cost if we are unable to generate an FMA instruction. So
a constrained fmuladd that can't be lowered to FMA will be lower the same way a
contrained mul followed by a constrained add would be.
Comment at: llvm/include/llvm/CodeGen/ISDOpcodes.h:355
+/// FMULADD/STRICT_FMULADD - A intermediate node, made functions handle
+/// constrained fmuladd the same as other constrained intrinsics.
Something is wrong with this comment. I'm not sure what it's trying to say but
the grammar is wrong.
After looking through the rest of the code, I think I understand what's going
on. I think we need a verbose comment to explain it. Here's my suggestion
```
FMULADD/STRICT_FMULADD -- These are intermediate opcodes used to handle the
constrained.fmuladd intrinsic. The FMULADD opcode only exists because it is
required for correct macro expansion and default handling (which is never
reached). There should never be a node with ISD::FMULADD. The STRICT_FMULADD
opcode is used to allow selectionDAGBuilder::visitConstrainedFPIntrinsic to
determine (based on TargetOptions and target cost information) whether the
constrained.fmuladd intrinsic should be lowered to FMA or separate FMUL and
FADD operations.
```
Having thought through that, however, it strikes me as a lot of overhead. Can
we just add special handling for the constrained.fmuladd intrinsic and make the
decision then to create either a STRICT_FMA node or separate STRICT_FMUL and
STRICT_FADD?
The idea that ISD::FMULADD is going to exist as a defined opcode but we never
intend to add any support for handling it is particularly bad.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D72820/new/
https://reviews.llvm.org/D72820
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
