[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:613
+    result = EmitUnpromotion(promotionTy, E->getSubExpr()->getType(), result);
+  return result;
+}
----------------
zahiraam wrote:
> rjmccall wrote:
> > You should unpromote only if we're not in a promoted context, which is to say, only if the `PromotionType` that was passed in is null.
> oh! right. The promotionTy is not even used in the function. Thanks.
I'm sorry, but this is not the change I requested; I meant you need to do this:

```
  QualType promotionTy = PromotionType.isNull() ? getPromotionType(E->getType()) : PromotionType;
  ComplexPairTy result = VisitMinus(E, promotionTy);
  if (PromotionType.isNull())
    result = EmitUnpromotion(promotionTy, result);
  return result;
```

The fact that you weren't seeing problems because of this makes me concerned.


================
Comment at: clang/test/CodeGen/X86/Float16-complex.c:1184
+// X86-NEXT:    store float [[NEG_I]], ptr [[RETVAL_IMAGP]], align 2
+// X86-NEXT:    [[TMP0:%.*]] = load <2 x half>, ptr [[RETVAL]], align 2
+// X86-NEXT:    ret <2 x half> [[TMP0]]
----------------
This code pattern is definitely wrong, and it's a sign that the expression evaluator returned the wrong type.  This is coercing a `_Complex float` into a `_Complex _Float16` through memory, which is essentially reinterpreting the first float as a pair of `_Float16`s.  You should go through your tests and make sure you don't see other instances of this.


================
Comment at: clang/test/Sema/Float16.c:13
-
-#ifdef HAVE
 _Complex _Float16 a;
----------------
I don't know why these test changes are in this patch.  My understanding is that we already committed a patch to enable `_Float16`, and this patch is just implementing the specialized emitter to avoid intermediate rounding when AVX512FP16 isn't available.

In the review of that other patch, I argued why this is the wrong change for this test.


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

https://reviews.llvm.org/D113107