ahatanak added inline comments.
================
Comment at: lib/CodeGen/CGExprScalar.cpp:997
- // Allow bitcast from vector to integer/fp of the same size.
- if (isa<llvm::VectorType>(SrcTy) ||
- isa<llvm::VectorType>(DstTy))
- return Builder.CreateBitCast(Src, DstTy, "conv");
+ if (isa<llvm::VectorType>(SrcTy) || isa<llvm::VectorType>(DstTy)) {
+ auto GetSize = [](const llvm::Type *Ty) {
----------------
bruno wrote:
> Please also add a comment explaining what's going on here, like we see for
> other snippets of logic above.
>
> It also sounds like this is more generic than it should (which can have
> unexpected side effects due to the lack of testcases covering vector with
> other element sizes). I suggest you either (a) add testcases for other sizes
> or (b) make the condition more restrictive to be sure you're only changing
> the logic you intend to (i.e., half and i16).
>
> After these changes, if it makes sense, can you refactor the logic under this
> condition into its own function? Seems like this function is too big already.
I'm surprised that we don't have many code-gen tests for vectors, but I can add
more test cases that are the same as the tests in fp16vec-ops.c except that the
element types are different.
Just to be clear, the only change I'm making here is to handle cases in which
the source and destination have different sizes. That shouldn't happen when for
example an i8 vector is converted to an i32 because if it did happen, that
would have previously caused an assertion when calling CreateBitCast (bitcast
requires the sizes of the destination and source be the same).
Also, I removed the lambda and instead used Type::getPrimitiveSizeInBit to
compute the vector size.
If this looks OK, I'll try refactoring this function.
================
Comment at: lib/Sema/SemaExpr.cpp:11433
ExprObjectKind OK = OK_Ordinary;
+ bool ConvertHalfVec = false;
----------------
bruno wrote:
> Assuming we're able to handle other vector types here, is it in
> `ConvertHalfVec` really necessary? It seems odd to me that we need to special
> case it in every case below.
ConvertHalfVec is needed to distinguish operations that involve vectors of half
from those that don't. If the operands are vectors of half, convertHalfVecBinOp
is called to promote the operands and truncate the result.
https://reviews.llvm.org/D32520
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
