rjmccall added inline comments.
================
Comment at: clang/lib/CodeGen/CGExprComplex.cpp:290
+ }
+ return PromotedTy;
+ }
----------------
Indentation is off
================
Comment at: clang/lib/CodeGen/CGExprComplex.cpp:305
+ result.second = Builder.CreateFPTrunc(result.second, ComplexElementTy,
\
+ "unpromotion");
\
+ }
\
----------------
Can you pull everything in this block out as a helper function? It could end
up being useful for other paths if they acquire specialized promoted emission.
Call it something like `EmitUnpromotion`.
================
Comment at: clang/lib/CodeGen/CGExprComplex.cpp:926
+ }
+ } else {
+ auto result = Visit(const_cast<Expr *>(E));
----------------
The `default` path in the `switch` above falls through, and because of this
`else` it skips directly to the end and doesn't emit anything. Test case would
be something like `f16c + (0, f16c)` — really, any binary operator besides the
four above that returns a complex value, such as comma, compound assignment, or
one of the C++ pointer-to-member operators.
================
Comment at: clang/lib/CodeGen/CGExprComplex.cpp:950
+ComplexExprEmitter::EmitPromotedComplexOperand(const Expr *E,
+ QualType PromotionType) {
+ if (E->getType()->isAnyComplexType()) {
----------------
Maybe call this the `OverallPromotionType` to make it clear that it's not
necessarily the right promotion type for the operand (as in fact it isn't in
the scalar case).
================
Comment at: clang/lib/CodeGen/CGExprComplex.cpp:953
+ if (!PromotionType.isNull())
+ return ComplexPairTy(CGF.EmitPromotedComplexExpr(E, PromotionType));
+ else
----------------
I think this conversion to `ComplexPairTy` is a no-op.
================
Comment at: clang/lib/CodeGen/CGExprComplex.cpp:1037
ComplexPairTy LHSVal = EmitLoadOfLValue(LHS, Loc);
OpInfo.LHS = EmitComplexToComplexCast(LHSVal, LHSTy, OpInfo.Ty, Loc);
} else {
----------------
This should do a conversion to the promoted LHS type.
================
Comment at: clang/lib/CodeGen/CGExprComplex.cpp:1050
+ OpInfo.LHS = ComplexPairTy(LHSVal, nullptr);
+ }
} else {
----------------
This should be testing equality with, and doing a conversion to, the promoted
`ComplexElementTy`.
================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3033
Expr *Op = E->getSubExpr();
+ QualType PromotionType = getPromotionType(Op->getType());
if (Op->getType()->isAnyComplexType()) {
----------------
If we don't want a promoted result, there's no point in promoting the operand
just to extract `_Real` or `_Imag`. But if the caller *does* want a promoted
result, then we should promote the operand before the extraction so that the
`_Real` / `_Imag` doesn't cause a truncation. So these methods should take a
promotion type and, if present, use it to emit the operand as promoted before
doing the extraction.
================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3089
-BinOpInfo ScalarExprEmitter::EmitBinOps(const BinaryOperator *E) {
+Value *ScalarExprEmitter::EmitPromoted(const Expr *E, QualType PromotionType) {
+ if (auto BO = dyn_cast<BinaryOperator>(E)) {
----------------
Please do `E = E->IgnoreParens();` as the first thing in this function, or else
parenthesizing an operand will cause intermediate truncations. You'll need to
do the same thing in the complex emitter.
================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3112
+ result = CGF.Builder.CreateFPExt(result, ConvertType(E->getType()));
+ return result;
+}
----------------
The first version of the fallback path (the one in the `else`) is correct.
Like with the complex emitter, though, you've got a bug where fall through from
the earlier `switch` ends up in the second path; test case is something like
`f16 + (true, f16)`.
The best fix is to just not try to use `else` for your fallback path. The
earlier blocks will return if they recognize cases that they can specially
handle; everything should fall out, and you handle everything that reaches that
point with the fallback path.
This code should check for `UnaryOperator` like it does for `BinaryOperator`
and delegate to your `_Real` / `_Imag` implementations above. You should
probably also handle unary `+` and `-` so that using those does not cause
intermediate truncations.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D113107/new/
https://reviews.llvm.org/D113107
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
