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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
