This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] Don't blow up in foldICmpWithCastAndCast on vector icmp instructions.
ClosedPublic

Authored by dneilson on Mar 2 2018, 6:01 PM.

Details

Summary

Presently, InstCombiner::foldICmpWithCastAndCast() implicitly assumes that it is
only invoked with icmp instructions of integer type. If that assumption is broken,
and it is called with an icmp of vector type, then it fails (asserts/crashes).

This patch addresses the deficiency. It allows it to simplify
icmp (ptrtoint x), (ptrtoint/c) of vector type into a compare of the inputs,
much as is done when the type is integer.

Diff Detail

Repository
rL LLVM

Event Timeline

dneilson created this revision.Mar 2 2018, 6:01 PM
anna added inline comments.Mar 5 2018, 8:03 AM
lib/Transforms/InstCombine/InstCombineCompares.cpp
3411 ↗(On Diff #136890)

Instead of capturing everything, let's capture just DL.

dneilson added inline comments.Mar 5 2018, 8:26 AM
lib/Transforms/InstCombine/InstCombineCompares.cpp
3411 ↗(On Diff #136890)

This is not capturing everything, it is only capturing the this pointer.

Per: http://en.cppreference.com/w/cpp/language/lambda

[&] captures all automatic variables **used in the body** of the lambda by reference and current object by reference if exists (emphasis mine).

There are no automatics used in the body of this lambda, so only the current object reference (i.e. the this pointer) is captured.

I could change [&] to [this] to make it more explicit, if desired...

anna added inline comments.Mar 5 2018, 9:05 AM
lib/Transforms/InstCombine/InstCombineCompares.cpp
3411 ↗(On Diff #136890)

But why do you need to capture this? Basically, this should do right:

const auto& CompatibleSizes = [&DL](Type* SrcTy, Type* DestTy) -> bool {
....

It makes it clearer that the only capture required is DL which is used in the lambda.

dneilson added inline comments.Mar 5 2018, 9:08 AM
lib/Transforms/InstCombine/InstCombineCompares.cpp
3411 ↗(On Diff #136890)

Non-obvious detail: DL is a member of the InstCombiner class, and not an automatic.

You can't capture members. Can only capture reference to current object (i.e. this), and automatics.

anna accepted this revision.Mar 5 2018, 9:27 AM

lgtm

lib/Transforms/InstCombine/InstCombineCompares.cpp
3411 ↗(On Diff #136890)

ah I didn't see that.

This revision is now accepted and ready to land.Mar 5 2018, 9:27 AM
This revision was automatically updated to reflect the committed changes.