[clang] Implement constexpr bit_cast for vectors

[DaMatrix]

This makes __builtin_bit_cast support converting to and from vector types in a constexpr context.

[zygoloid]

Generally this looks good to me, but doesn't seem to properly handle __attribute__((ext_vector_type(N))) bool.

[DaMatrix]

While attempting to make the result properly endianness-dependent, I noticed that VectorExprEvaluator::VisitCastExpr with CK_BitCast already had code for this which takes the byte ordering into account (although it also didn't handle bool vectors correctly either). I've now moved the existing Vector<->APInt bit casting code into a pair of separate methods, and made both CK_BitCast and the new bit casting code use those.

[zygoloid]

Thanks, I like the code reuse! There are some minor optimization and simplification opportunities in the code you're reusing that might be worth addressing now.

I'm worried that vectors with padding (which I think is only vectors of x86 long double) might not be being treated properly by bit_cast here. It's probably a bit awkward to fix, given the new approach, but maybe we can handle the padding separately from the conversion of the vector to/from APInt.

[zygoloid]

Thanks, looks good. Just a couple of minor suggestions.

[zygoloid]

Looks good, other than the handling of x86_fp80, which doesn't seem to match Clang's current runtime behavior.

Review of older changes

[zygoloid]

Thanks, this looks good to me other than a small test coverage (see comment) / diagnostic wording issue.

(For future reference, please don't force-push to PR branches; that makes it much harder for your reviewer to see what's changed since their last review and can cause github to lose track of comments. See https://llvm.org/docs/GitHub.html#updating-pull-requests)

[zygoloid]

Please add some test coverage that we properly diagnose this situation.

I think we'll produce a diagnostic mentioning bit_cast, which is probably not what we want here. Maybe the simplest thing to do to fix that would be to replace bit_cast with bit cast in all the diagnostics. That seems at least as good for the std::bit_cast / __builtin_bit_cast case, and better for the vector cast case.

[DaMatrix]

Interestingly, uncommenting these lines doesn't cause any diagnostic messages to be generated, only an error stating error: initializer element is not a compile-time constant. I'll see if I can get the diagnostic to be output and change the messages as suggested.

[DaMatrix]

So, on further investigation, it looks like the diagnostic isn't being emitted at all because the constant evaluation code is called with an EvalInfo which has diagnostics disabled, and is never called again.

https://github.com/llvm/llvm-project/blob/72f63b695c9ebd9c7032c4b754ff7965c28fad5c/clang/lib/AST/Expr.cpp#L3486
https://github.com/llvm/llvm-project/blob/72f63b695c9ebd9c7032c4b754ff7965c28fad5c/clang/lib/AST/ExprConstant.cpp#L15483-L15485

And once it's been evaluted with no diagnostic, an diag::err_init_element_not_constant is emitted which triggers an error and so it never gets evaluated again with diagnostics enabled.

https://github.com/llvm/llvm-project/blob/72f63b695c9ebd9c7032c4b754ff7965c28fad5c/clang/lib/Sema/SemaDecl.cpp#L12510-L12514

What should I do here? This feels like a weakness of the code in Sema; any changes I make here are going to break a bunch of tests which expect no diagnostics to be output in these cases.

[zygoloid]

Perhaps you could add a C++ testcass with the variable declared constexpr.

[zygoloid]

Thanks, LGTM

[DaMatrix]

@zygoloid since no one else has added any comments, can you commit this? I don't have write access to the repository.