zahiraam marked 7 inline comments as done.
zahiraam added inline comments.
================
Comment at: clang/lib/CodeGen/CGExprComplex.cpp:896
+
+ComplexPairTy ComplexExprEmitter::EmitPromoted(const Expr *E) {
+ if (auto *BinOp = dyn_cast<BinaryOperator>(E->IgnoreParens())) {
----------------
pengfei wrote:
> rjmccall wrote:
> > pengfei wrote:
> > > 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`.
> > Well, you can provide operation-by-operation lowering support if you want
> > to, but what's going to come out of Clang IRGen in this mode is not going
> > to rely on it. It's far easier to just emit these operations differently,
> > always using `float`, than to try to recognize the special cases where all
> > the inputs are naturally `half` and so you might as well emit a `half`
> > operation.
> >
> > And I think the postcondition that `half` operations don't come out of
> > IRGen is a generally useful property even if you've already written this
> > lowering code. Among other things, it makes it easy to test that you
> > haven't missed a case where you should be doing promoted emission.
> We define `_Float16` as ABI type and require targets to support the ABI
> lowering. See
> https://clang.llvm.org/docs/LanguageExtensions.html#half-precision-floating-point
> Promoting in FE doesn't mean it works on any targets by default, so I think
> it's not much useful.
> We need to support `-fexcess-precision=16` the same as GCC does, it's easier
> to do nothing than promote and then truncate each of the operations in FE.
> I think in general, lowering operations for a type is a business of backend.
> We still need backend to do so for other FEs even we do it in Clang. A recent
> example is BF16, which people chose to lower in backend rather than mlir, see
> D126444.
>
> > Among other things, it makes it easy to test that you haven't missed a case
> > where you should be doing promoted emission.
> I don't understand, how can we check the missed cases if the promotion was
> done in FE?
This patch is doing by default what GCC is doing with the
-fexcess-precision=16. And that would be for expressions with any number of
operators. But it looks like you want promotion/truncation only for expression
with more than one operator? I think I agree with @rjmccall that it is easier
done this way than trying to recognize the special cases.
What you want is for a+b to generate fadd half (not extension, and no
truncation ) and for a+b*c to generated a set of fpext, fadd float and fptrunc?
Does GCC use their option for special cases too? I would think that when this
option is used all expression (regardless of the number of operators) are
extended/truncated right?
I am not sure I like the idea of lowering some cases in the FE and some others
in the BE. I think it would be cleaner to decide that lowering be done in FE
for all expressions and have BE react to it.
@rmjccall not sure what you mean by:
"And I think the postcondition that half operations don't come out of IRGen is
a generally useful property even if you've already written this lowering code.
Among other things, it makes it easy to test that you haven't missed a case
where you should be doing promoted emission."
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
