This is an archive of the discontinued LLVM Phabricator instance.

Fix ICE on lowering of constexpr vector splats
ClosedPublic

Authored by george.burgess.iv on Nov 20 2015, 11:04 AM.

Details

Summary

When evaluating constexpr vector splats, we weren't doing appropriate type conversions on the literal we were splatting, causing assertion failures in cases like:

void foo(vector float FloatInput, vector short ShortInput) {
  (void)(FloatInput == (vector float)0); // OK
  (void)(ShortInput == (vector short)0); // OK

  constexpr vector float Floats = (vector float)0;
  (void)(FloatInput == Floats); // ICE -- fcmp between [4 x i32] and [4 x f32]

  constexpr vector short Shorts = (vector short)0;
  (void)(ShortInput == Shorts); // ICE -- fcmp between vec of i16 and vec of i32
}

(The same issue applied for cases like (vector short)0; it would be lowered as a vector of i32.)

This patch fixes these in ExprConstant rather than CodeGen, because it allows us to more sanely model overflow/complain to the user/... in the evaluator.

This patch also contains a few generic code cleanliness changes. I'm happy to drop any/all of them if we decide they're not helpful. :)

Diff Detail

Event Timeline

george.burgess.iv retitled this revision from to Fix ICE on lowering of constexpr vector splats.
george.burgess.iv updated this object.
george.burgess.iv added a reviewer: rsmith.
george.burgess.iv added a subscriber: cfe-commits.
rsmith edited edge metadata.Nov 21 2015, 11:11 AM
rsmith added a subscriber: rsmith.

It would seem cleaner to build an ImplicitCastExpr node in Sema between the
operand and the splat node.

majnemer added inline comments.
lib/CodeGen/CGExprConstant.cpp
1364–1365

Is this unreachable for vectors of pointer type?

george.burgess.iv edited edge metadata.

We now add implicit casts to splatted literals, as Richard suggested, instead of trying to handle this as a special case in ExprConstant.

lib/CodeGen/CGExprConstant.cpp
1364–1365

Might you have an example of how to use a vector of pointers in clang? My knowledge of vectors as a whole is super limited, so I'm probably missing something obvious, but I can't seem to get any of the vector types mentioned here http://clang.llvm.org/docs/LanguageExtensions.html#vectors-and-extended-vectors to accept a pointer element type, nor is my grep-fu good enough to find e.g. a vector (int*) in test/.

Changes to ExprConstant and CGExprConstant appear to be pure cleanups; please check those in as a separate change.

When this is done, you should also be able to remove the IgnoreImpCasts and EmitScalarConversion calls in the CK_VectorSplat handling in ScalarExprEmitter::VisitCastExpr.

lib/Sema/SemaExpr.cpp
5610

Looking at ScalarExprEmitter::VisitCastExpr, it seems like we are supposed to do something slightly bizarre if the source type is bool and we're in OpenCL mode -- in that case we're supposed to convert true to -1 instead of 1. In order for ExprConstant to get that case right, we should emit the appropriate implicit cast for that conversion here -- maybe add a CK_SignedBooleanToIntegral and a CK_SignedBooleanToFloating; I don't see any nice way to express this with our existing cast kinds.

george.burgess.iv marked an inline comment as done.

Changes to ExprConstant and CGExprConstant appear to be pure cleanups; please check those in as a separate change

r255314, thanks.

When this is done, you should also be able to remove the IgnoreImpCasts and EmitScalarConversion calls in the CK_VectorSplat handling in ScalarExprEmitter::VisitCastExpr.

Yup!

Aside: I can't figure out how to get c++11 constexpr evaluation in to work with OpenCL (vector expressions apparently aren't considered ICEs in non-c++11 mode). So, the bit of special code in ExprConstant is untested at the moment. I'm open to suggestions for how to test it; I tried things like int arr[((int4)true)[0] == -1 ? 1 : -1];/tricks with enable_if, but wasn't quite able to get it to do what I wanted. :)

lib/Sema/SemaExpr.cpp
5610

...And this behavior is only present when splatting. Joy.

TIL you can use OpenCL vectors in C++ files. Fixed a bug related to not knowing this + added tests for the constexpr cases.

Also, is there a better way to do the bool -> APFloat conversion in ExprConstant, lines 8108-8110? I'm assuming that float semantics matter, otherwise Result = APFloat(-BoolResult) seems nicer.

rsmith added a comment.Jan 6 2016, 5:58 PM

Maybe we could remove CK_BooleanToSignedFloating and model that conversion as a sequence of CK_BooleanToSignedIntegral followed by CK_IntegralToFloating? I don't imagine it's a common operation, so the simpler AST representation is probably worth more than the minor memory savings.

lib/AST/ExprConstant.cpp
7794

I think this will produce the wrong value when the destination type of the cast is __int128. We should probably fix this by making ASTContext::MakeIntValue sign-extend if necessary when the target type is signed.

george.burgess.iv marked an inline comment as done.

Addressed all feedback, and added tests for bool -> [n x i128] case.

Maybe we could remove CK_BooleanToSignedFloating and model that conversion as a sequence of CK_BooleanToSignedIntegral followed by CK_IntegralToFloating? I don't imagine it's a common operation, so the simpler AST representation is probably worth more than the minor memory savings.

SGTM.

george.burgess.iv marked 2 inline comments as done.Jan 12 2016, 3:09 PM

Ping :)

rsmith accepted this revision.Jan 12 2016, 5:08 PM
rsmith edited edge metadata.
rsmith added inline comments.
lib/CodeGen/CGExprScalar.cpp
815–816

The TypePtr can still have qualifiers in it in some cases. Use ASTContext::hasSameType.

This revision is now accepted and ready to land.Jan 12 2016, 5:08 PM
This revision was automatically updated to reflect the committed changes.