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

[Phoebe Wang via Phabricator via cfe-commits]

pengfei added inline comments.


================
Comment at: clang/docs/ReleaseNotes.rst:491
 
+- Support for ``AVX512-FP16`` instructions has been added.
+- Support for ``_Float16`` type has been added.
----------------
This line doesn't need anymore.


================
Comment at: clang/lib/Basic/Targets/X86.cpp:242
       HasAVX512FP16 = true;
       HasFloat16 = true;
+      HasLegalHalfType = true;
----------------
`HasFloat16 = true` can be removed. `+avx512fp16` implies `+avx512f` thus `SSELevel >= SSE2`


================
Comment at: clang/lib/CodeGen/CGExprComplex.cpp:896
+
+ComplexPairTy ComplexExprEmitter::EmitPromoted(const Expr *E) {
+  if (auto *BinOp = dyn_cast<BinaryOperator>(E->IgnoreParens())) {
----------------
rjmccall wrote:
> zahiraam wrote:
> > rjmccall wrote:
> > > `EmitPromoted` should take the promotion type.
> > > 
> > > You are missing the logic in this file which handles *unpromoted* emission (where you have a binary operator but the context wants an unpromoted value) by recognizing that you need to do a promoted operation and then truncate.
> > Sorry but not sure what you mean here. A case where we don't want any promotion would be:
> > 
> > float _Complex add(float _Complex a, float _Complex b) {
> >   return a + b;
> > }
> > 
> > In this case, getPromotionType would return the null type and EmitBinOps would just go through the  "older" control path where there is no promotion.
> > Unless I misunderstood your comment?
> > 
> > 
> I'm talking about the unpromoted emission path for an operation that we want to *internally* promote.  So like your example but using `_Float16` — we want to emit the binary `+` as a `float` operation, but the context needs an unpromoted value because it's going to be returned.
> 
> The promoted emission path (the various `EmitPromoted` methods) represents a request by the caller to produce a result that doesn't match the formal type of the expression.  The normal emission path `Visit` etc.) represents a request by the caller to produce a result normally, i.e. one that matches the formal type.  In general, we always start in the latter because arbitrary contexts always expect a value of the formal type; it's only these recursive calls within promoted emitters that contextually want a promoted value.
> 
> In your current patch, you're entering the promoted path by special-casing one context that frequently terminates promoted emission: `EmitComplexExprIntoLValue`, which is used for things like assignment.  That's not the right way to handle it, though.  Instead, you should do like I asked you to do with the scalar emitter, which is to recognize in the normal emission path for a promotable operation (like binary `+`) that you need to promote the operation and then unpromote the result.  Then things like `EmitComplexExprIntoLValue` will continue to simply enter the normal path because that's the kind of value they need, and the promotion logic will do the right thing from there to ensure we don't emit a `_Float16` operation in LLVM IR.
> 
> Incidentally, I thought of another context that you ought to do promotion for: when we're converting a promotable value to a larger floating-point type, we should presumably convert the promoted value directly rather than truncating it to the unpromoted type before the conversion.
I'm not familiar with the FE details, but I understand the background is just to promote expressions that have more than one operation. So I agree with @zahiraam, and
> to ensure we don't emit a _Float16 operation in LLVM IR
is not what we expected here.

`_Float16` is a ABI type for which a target supported has to lower any `_Float16` operations in LLVM IR. See what we have done on X86 backend by D107082.

That said, a single `a + b` can be and should be emitted as `%c = fadd half %a, %b`.


================
Comment at: clang/test/CodeGen/X86/Float16-arithmetic.c:11-15
+  // CHECK: fpext half {{.*}} to float
+  // CHECK: load half, ptr {{.*}}
+  // CHECK: fpext half {{.*}} to float
+  // CHECK: fadd float {{.*}}, {{.*}}
+  // CHECK: fptrunc float {{.*}} to half
----------------
So what we expected is emitting `fadd half {{.*}}, {{.*}}` only. The same for mul and div.


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

https://reviews.llvm.org/D113107