[PATCH] D113107: Support of expression granularity for _Float16.
John McCall via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Wed Aug 10 17:26:27 PDT 2022
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
More information about the cfe-commits
mailing list