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

[John McCall via Phabricator via llvm-commits]

rjmccall added a comment.

Somehow we've taken a huge step back on unpromotion, and I'm worried you're now doing the exact thing I didn't want us doing and forcing all the downstream clients to handle the possibility of a promoted result.



================
Comment at: clang/lib/CodeGen/CGExprComplex.cpp:418
+    EmitStore(Val, lvalue.getAddress(CGF), lvalue.getType(),
+              lvalue.isVolatileQualified());
 }
----------------
You should not be changing this code; clients should be expected to give you a value that matches the type of the l-value, which generally means they need to be unpromoting.


================
Comment at: clang/lib/CodeGen/CGExprComplex.cpp:615
+  ComplexPairTy result = VisitMinus(E, promotionTy);
+  return result;
+}
----------------
This is not unpromoting if the original `PromotionType` is null.


================
Comment at: clang/lib/CodeGen/CGExprComplex.cpp:613
+    result = EmitUnpromotion(promotionTy, E->getSubExpr()->getType(), result);
+  return result;
+}
----------------
rjmccall wrote:
> zahiraam wrote:
> > rjmccall wrote:
> > > You should unpromote only if we're not in a promoted context, which is to say, only if the `PromotionType` that was passed in is null.
> > oh! right. The promotionTy is not even used in the function. Thanks.
> I'm sorry, but this is not the change I requested; I meant you need to do this:
> 
> ```
>   QualType promotionTy = PromotionType.isNull() ? getPromotionType(E->getType()) : PromotionType;
>   ComplexPairTy result = VisitMinus(E, promotionTy);
>   if (PromotionType.isNull())
>     result = EmitUnpromotion(promotionTy, result);
>   return result;
> ```
> 
> The fact that you weren't seeing problems because of this makes me concerned.
This is not unpromoting if the original `PromotionType` is null.


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


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

https://reviews.llvm.org/D113107