This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] [NFC] Add tests for peeking through FP casts for sign-bit compares (PR36682)
ClosedPublic

Authored by lebedev.ri on Mar 12 2018, 8:49 AM.

Details

Summary

This pattern came up in PR36682:
https://bugs.llvm.org/show_bug.cgi?id=36682
https://godbolt.org/g/LhuD9A

Tests for proposed fix in D44367.

Looking at the IR pattern in question, as per alive-nj, for all the type combinations i checked
(input: i16, i32, i64; intermediate: half/i16, float/i32, double/i64)
for the following icmp comparisons the sitofp+bitcast can be dropped:

  • eq 0
  • ne 0
  • slt 0
  • sle 0
  • sge 0
  • sgt 0
  • slt 1
  • sge 1
  • sle -1
  • sgt -1

I did not check vectors, but i'm guessing it's the same there.

Thus all these cases are in the testcase (along with the vector variant with additional undef element in the middle).
There are no negative patterns here (unless alive-nj lied/is broken), all of these should be optimized.

Generated with

Diff Detail

Repository
rL LLVM

Event Timeline

lebedev.ri created this revision.Mar 12 2018, 8:49 AM
spatel added inline comments.Mar 12 2018, 9:04 AM
test/Transforms/InstCombine/cast-int-icmp-eq-0.ll
89–100 ↗(On Diff #138023)

As the current check line shows, this verifies that we canonicalize the predicate to 'slt'. Once that canonicalization is made, the test is identical to a later test (see below), so I don't think it's worth duplicating this or similar tests with sle/sge predicates. Remove all of those?

define i1 @i32_cast_cmp_slt_int_1_sitofp_float(i32 %i) {
; CHECK-LABEL: @i32_cast_cmp_slt_int_1_sitofp_float(
; CHECK-NEXT:    [[F:%.*]] = sitofp i32 [[I:%.*]] to float
; CHECK-NEXT:    [[B:%.*]] = bitcast float [[F]] to i32
; CHECK-NEXT:    [[CMP:%.*]] = icmp slt i32 [[B]], 1
; CHECK-NEXT:    ret i1 [[CMP]]
;
  %f = sitofp i32 %i to float
  %b = bitcast float %f to i32
  %cmp = icmp slt i32 %b, 1
  ret i1 %cmp
}
spatel added a subscriber: nlopes.Mar 12 2018, 9:11 AM

cc @nlopes because the online version of Alive at https://rise4fun.com/Alive/ doesn't recognize 'sitofp' as a valid opcode.

cc @nlopes because the online version of Alive at https://rise4fun.com/Alive/ doesn't recognize 'sitofp' as a valid opcode.

Also see https://github.com/nunoplopes/alive/issues/41#issuecomment-372133708

lebedev.ri updated this revision to Diff 138043.EditedMar 12 2018, 10:03 AM
lebedev.ri marked an inline comment as done.

Removed 4 patterns:

  • sle 0 which is canonicalized to slt 1, so don't test it.
  • sge 0 which is canonicalized to sgt -1, so don't test it.
  • sge 1 which is canonicalized to sgt 0, so don't test it.
  • sle -1 which is canonicalized to slt 0, so don't test it.

(those canonical variants are already tested here.)

spatel accepted this revision.Mar 12 2018, 10:33 AM

LGTM.

This revision is now accepted and ready to land.Mar 12 2018, 10:33 AM
This revision was automatically updated to reflect the committed changes.