This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] peek through unsigned FP casts for zero-equality compares (PR36682)
ClosedPublic

Authored by lebedev.ri on Mar 13 2018, 6:20 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

lebedev.ri created this revision.Mar 13 2018, 6:20 AM

Just use isEquality().

Rebased ontop of updated tests.

This comment was removed by lebedev.ri.
spatel accepted this revision.Mar 18 2018, 7:41 AM

LGTM. See inline for a couple of nits.

lib/Transforms/InstCombine/InstCombineCompares.cpp
4516

Don't need braces for the single-line 'if' clause.

test/Transforms/InstCombine/cast-unsigned-icmp-eqcmp-0.ll
4

Remove unnecessary comment line.

This revision is now accepted and ready to land.Mar 18 2018, 7:41 AM

Thank you for the review!

lib/Transforms/InstCombine/InstCombineCompares.cpp
4516

It occupies more than one line though.
But ok, if you insist.

test/Transforms/InstCombine/cast-unsigned-icmp-eqcmp-0.ll
4

For future reference, could you please clarify *when* target datalayout should be specified in tests?

spatel added inline comments.Mar 18 2018, 7:50 AM
lib/Transforms/InstCombine/InstCombineCompares.cpp
4516

It's a fuzzy rule, so I was just going by the formatting of the code above this.

test/Transforms/InstCombine/cast-unsigned-icmp-eqcmp-0.ll
4

Sure - it should be specified when it affects the outcome. For instcombine, this would be when endianness or type legality is used to determine if/how a transform fires. So you'd see some query of the datalayout in the code.

Note that in this case, the line is commented out, so it definitely doesn't add anything to the test....unless I've missed some reason for it to be specified as a comment?

lebedev.ri added inline comments.Mar 18 2018, 7:54 AM
lib/Transforms/InstCombine/InstCombineCompares.cpp
4516

Ok, consistency reason is good.

test/Transforms/InstCombine/cast-unsigned-icmp-eqcmp-0.ll
4

Note that in this case, the line is commented out, so it definitely doesn't add anything to the test....

Oops, i kinda missed that :)

unless I've missed some reason for it to be specified as a comment?

Not that i know of.

Ok, will drop it.

This revision was automatically updated to reflect the committed changes.
This comment was removed by lebedev.ri.