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

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: clang/lib/CodeGen/CGExprComplex.cpp:964
   TestAndClearIgnoreImag();
   BinOpInfo Ops;
 
----------------
rjmccall wrote:
> Please make a helper function to emit an operand as a possibly-promoted complex value.
As requested, please introduce this helper function so that here you can just write:
```
Ops.LHS = EmitPromotedComplexOperand(E->getLHS(), PromotionTy);
Ops.RHS = EmitPromotedComplexOperand(E->getRHS(), PromotionTy);
```

`EmitPromotedComplexOperand` will do what's being done here for each side:
```
if (!E->getType()->isAnyComplexType()) {
  return CGF.EmitPromotedScalarExpr(E, PromotionType);
} else {
  return CGF.EmitPromotedComplexExpr(E, PromotionType);
}
```

This would rely on making those functions check for a null PromotionType and just fall back to the normal emission path.  Doing that seems it would simplify a lot of code.


================
Comment at: clang/lib/CodeGen/CGExprComplex.cpp:295
+            CGF.ConvertType(                                                   \
+                E->getType()->castAs<ComplexType>()->getElementType()),        \
+            "unpromotion");                                                    \
----------------
If you pull `CGF.ConvertType(E->getType()->castAs<ComplexType>()->getElementType())` out into a helper function, and call it once for both lanes, this will get so much more concise.


================
Comment at: clang/lib/CodeGen/CGExprComplex.cpp:932
+    return ComplexPairTy(Resultr, Resulti);
+  }
+  // fallback path
----------------
This whole `else` block is identical to the fallback path beneath it.


================
Comment at: clang/lib/CodeGen/CGExprComplex.cpp:940
+      Result.second,
+      CGF.ConvertType(E->getType()->castAs<ComplexType>()->getElementType()));
+  return ComplexPairTy(Resultr, Resulti);
----------------
Like above, please convert the type once for both lanes.  Also, I think you might need to handle null values, because emitters can generate null results if the result is unused.


================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:827
+  Value *VisitBin##OP##Assign(const CompoundAssignOperator *E) {               \
+    QualType promotionTy = getPromotionType(E);                                \
+    auto result =                                                              \
----------------
Unfortunately, this won't work because the type of the assignment expression is not necessarily the type of the compound operation.  Also, in C the result of an assignment is the r-value that was actually stored, and in C++ it's an l-value referring to the LHS; in either case, promoted emission can never propagate through the expression.  So you should just handle promotion internally to EmitCompoundAssign, rather than passing in a promotion type, and it will always be expected to return an unpromoted result.


================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3117
+    Result.Ty  = E->getType();
+  }
   Result.Opcode = E->getOpcode();
----------------
This is one of those places that would get simplified if we just made `EmitPromotedScalarExpr` handle the case of a null promotion type by falling back to the normal path.  All these functions that are used from both the promoted and normal paths find themselves doing this.


================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3132
     return CGF.EmitScalarCompoundAssignWithComplex(E, Result);
 
   // Emit the RHS first.  __block variables need to have the rhs evaluated
----------------
So, I think what you want to do here is basically compute `getPromotedType` of all the individual pieces that go into this: the computation result type, the computation LHS type, and the RHS type.  
- `OpInfo.Ty` will be the promoted computation result type, if it exists, or else the normal one.
- Call `EmitPromoted` on the RHS using the promoted RHS type.
- Convert the loaded LHS to the promoted computation LHS type, if it exists, or else the normal one.

Then you do the operation and convert from `OpInfo.Ty` back to the LHS type before you do the assignment.


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

https://reviews.llvm.org/D113107