Page MenuHomePhabricator

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

Repository
rL LLVM

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
1362–1363 ↗(On Diff #40798)

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
1362–1363 ↗(On Diff #40798)

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
5576 ↗(On Diff #40895)

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
5576 ↗(On Diff #40895)

...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 ↗(On Diff #42686)

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
816–817 ↗(On Diff #44187)

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.