[PATCH] D113107: Support of expression granularity for _Float16.

[John McCall via Phabricator via llvm-commits]

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