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

[Zahira Ammarguellat via Phabricator via llvm-commits]

zahiraam added inline comments.


================
Comment at: clang/lib/CodeGen/CGExprComplex.cpp:950
+ComplexExprEmitter::EmitPromotedComplexOperand(const Expr *E,
+                                               QualType PromotionType) {
+  if (E->getType()->isAnyComplexType()) {
----------------
rjmccall wrote:
> 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).
Changed it, but not sure it makes it clearer.


================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3112
+    result = CGF.Builder.CreateFPExt(result, ConvertType(E->getType()));
+  return result;
+}
----------------
rjmccall wrote:
> 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.
OK. Fixed the fallback part. But I can't find a test case that would make me land into the switch for UnaryOperator . The code I added is not exercised. Would you mind suggesting a test case? Not sure about the f16 + (true, f16) above?



CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113107/new/

https://reviews.llvm.org/D113107