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

[John McCall via Phabricator via llvm-commits]

rjmccall added a comment.

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

> I think I introduced a bug when replacing the VisitUnaryMinus/Plus/Imag/Real with VisitMinus/Plus/Imag/Real. Now this simple test case is failing in the non-promotion path (with the +avx512fp16).
>
> _Float16 _Complex MinusOp_c_c(_Float16 c) {
>
>   return -c;
>
> }
>
> error: cannot compile this scalar expression yet
>
>   return -c;
>          ^~
>
> 1 error generated.
>
> The way I implemented it is that I have added the HANDLE_UNOP macro but I think that's wrong. I think I need to keep the VisitUnary* methods and fork out of them in the promotion path!  Same for both Scalar and Complex Exprs. Your thoughts?

Right, you still need to declare `VisitUnary*` methods that the CRTP visitor will call.  But you can avoid the boilerplate of having separate methods by just adding the `PromotionType` argument to `VisitUnary*` with a default argument that's appropriate for the normal path:

  llvm::Value *VisitUnaryMinus(const UnaryOperator *E, QualType PromotionType = QualType());



================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3140
+      return CGF.Builder.CreateFPExt(result, ConvertType(E->getType()));
+  }
+  return result;
----------------
zahiraam wrote:
> rjmccall wrote:
> > Please extract this block out as:
> > 
> > ```
> > llvm::Value *EmitPromotedValue(llvm::Value *result, QualType PromotionType);
> > ```
> These changes you are proposing is when the argument of the unary __imag / __real is of type _Complex Float16. I would think that this new method EmitPromotedValue would be replacing the equivalent code in ComplexEmitter::EmitPromoted instead,  not in the scalar emitter, right?
I probably mixed up which emitter I commented on.  The upshot is that I would like there to be `EmitPromotedValue` and `EmitUnpromotedValue` helper functions on both emitters (which of course would take/return an `llvm::Value*` on the scalar emitter and a `CGComplexPair` on the complex emitter), just so that we have all the value promotion/unpromotion logic for each emitter in one place.


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

https://reviews.llvm.org/D113107