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

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Thanks, this is looking very close now.  Please handle the unary minus case in the complex emitter as well; I think that's all we need in terms of basic features.



================
Comment at: clang/lib/CodeGen/CGExprComplex.cpp:296
+  ComplexPairTy EmitUnpromotion(QualType promotionTy, const BinaryOperator *E,
+                                ComplexPairTy &result) {
+    if (!promotionTy.isNull()) {
----------------
Please take `result` by value instead of by reference; it's surprising that this both returns the value and modifies the parameter.


================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:811
+      if (ElementType->isFloat16Type())
+        return CGF.getContext().getComplexType(CGF.getContext().FloatTy);
+    }
----------------
This path needs to be gated by `shouldEmitFloat16WithExcessPrecision()`.


================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3059
+      return CGF.EmitLoadOfLValue(CGF.EmitLValue(E), E->getExprLoc())
+          .getScalarVal();
+    }
----------------
Please do something like:

```
llvm::Value *result = CGF.EmitComplexExpr(Op, /*IgnoreReal*/IgnoreResultAssign, /*IgnoreImag*/true).first;
if (result && !PromotionType.isNull())
  result = EmitPromotedValue(result);
return result;
```


================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3082
+      return CGF.EmitLoadOfLValue(CGF.EmitLValue(E), E->getExprLoc())
+          .getScalarVal();
+    }
----------------
Uh.  The existing code here is just wrong; I wonder if it's never been tested.  `_Imag` is not a commonly-used operator, especially on l-values.

Please do something like:

```
  llvm::Value *result = CGF.EmitComplexExpr(Op, /*IgnoreReal*/true, /*IgnoreImag*/IgnoreResultAssign).second;
  if (result && !PromotionType.isNull())
    result = EmitPromotedValue(result);
  return result;
```


================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3140
+      return CGF.Builder.CreateFPExt(result, ConvertType(E->getType()));
+  }
+  return result;
----------------
Please extract this block out as:

```
llvm::Value *EmitPromotedValue(llvm::Value *result, QualType PromotionType);
```


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

https://reviews.llvm.org/D113107