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<llvm::CallBase>(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
