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

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3112
+    result = CGF.Builder.CreateFPExt(result, ConvertType(E->getType()));
+  return result;
+}
----------------
zahiraam wrote:
> 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?
> 
In general, you test the promoted path by using the operator in a context that wants to promote its operands.  So you would test a unary operator by doing something like `f16 + -f16`: the operator is the operand of a Float16 `+`, which does promotion.


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

https://reviews.llvm.org/D113107