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

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D113107#3585672 <https://reviews.llvm.org/D113107#3585672>, @zahiraam wrote:

> @rjmccall 
> The comma && ternary cases for EmitPromoted, I am not sure what needs to happen there?

Basically just the same thing that the normal paths do, except you recurse with `EmitPromoted` instead of `Visit`.  For the ternary operator, you'll need to build the PHINodes for the promoted type instead of the expression type.

> I am also not quite sure about the ComplexExpr’s generated IR . I updated only some of the Float16-Complex.c tests but before editing the rest of the tests, I would like your feedback to see if the IR generated is correct (mul_half_cc is now generating __mulsc3 with truncation instead of __mulhc3).

That seems correct, yeah?  You want float semantics instead of half semantics when you're doing this.  Presumably the runtime shouldn't have to provide the full support library for half when you're emitting this way.



================
Comment at: clang/include/clang/Basic/TargetInfo.h:910
+    return false;
+  }
+
----------------
Please name this `shouldEmitFloat16WithExcessPrecision`


================
Comment at: clang/lib/CodeGen/CGExprComplex.cpp:72-74
+    if (E->getType()->isComplexType()) {
+      QualType ElementType =
+          E->getType()->castAs<ComplexType>()->getElementType();
----------------
But I think you can actually assert that you're only dealing with a complex-typed expression here; this is the complex expr emitter, after all.


================
Comment at: clang/lib/CodeGen/CGExprComplex.cpp:896
+
+ComplexPairTy ComplexExprEmitter::EmitPromoted(const Expr *E) {
+  if (auto *BinOp = dyn_cast<BinaryOperator>(E->IgnoreParens())) {
----------------
`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.


================
Comment at: clang/lib/CodeGen/CGExprComplex.cpp:940
+                                                       llvm::Type *DstType,
+                                                       SourceLocation Loc) {
+  llvm::Type *SrcElementTy;
----------------
I don't think you want either of these functions.  They're at the very least named wrong, since they're not actually emitting expressions; they're just doing conversions.  But really they're just doing FPExt/FPTrunc on a value.


================
Comment at: clang/lib/CodeGen/CGExprComplex.cpp:949
+    assert("We are not expecting to land here, since we are promoting to a "
+           "larger type size");
+  else {
----------------
Is this really the accepted way to check in IR whether one floating-point type is smaller than another?

Even if it, I think the canonical way to assert this would be:
```
assert(DstElementTy->getTypeID() > SrcElementTy->getTypeID() && "promoting to a smaller type size?");
```


================
Comment at: clang/lib/CodeGen/CGExprComplex.cpp:964
   TestAndClearIgnoreImag();
   BinOpInfo Ops;
 
----------------
Please make a helper function to emit an operand as a possibly-promoted complex value.


================
Comment at: clang/lib/CodeGen/CGExprComplex.cpp:969
+      Ops.LHS = CGF.EmitPromotedScalarExpr(
+          CGF.EmitScalarExpr(E->getLHS()),
+          E->getType(), CGF.ConvertType(
----------------
No, this should look like:

```
Ops.LHS = CGF.EmitPromotedScalarExpr(E->getLHS(), PromotionType->castAs<ComplexType>()->getElementType());
```

and then that function will be in `CGExprScalar.cpp` and will call `EmitPromoted` using a scalar emitter.

Similarly, you should have an `EmitPromotedComplexExpr` in this file which calls `EmitPromoted` using a complex emitter, and then `ScalarExprEmitter::EmitPromoted` will use that when emitting the `_Real`/`_Imag` unary operators.


================
Comment at: clang/lib/CodeGen/CGExprComplex.cpp:1260
+                                       "unpromotion");
+  }
   Emitter.EmitStoreOfComplex(Val, dest, isInit);
----------------
You should just not be emitting promoted here.


================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:822
+      result = Builder.CreateFPTrunc(result, ConvertType(E->getType()),    \
+                                     "unpromotion");                       \
+    return result;                                                         \
----------------
Style nit: missing indentation


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

https://reviews.llvm.org/D113107