Page MenuHomePhabricator

[InstSimplify] fold srem instruction if its two operands are negatived.
ClosedPublic

Authored by shchenz on Jul 17 2018, 6:05 AM.

Details

Summary

This is from code review comment from D49382. We already finish related optimization for add instruction in D49216 and sdiv instruction in D49382.

This patch is for srem instruction. for example:

define i32 @negated_operand(i32 %x) {
  %negx = sub i32 0, %x
  %rem = srem i32 %negx, %x
  ret i32 %rem 
}

can be folded to:

define i32 @negated_operand(i32 %x) {
  ret i32 0
}

Alive verification:
https://rise4fun.com/Alive/q20
https://rise4fun.com/Alive/h4o

Diff Detail

Repository
rL LLVM

Event Timeline

shchenz created this revision.Jul 17 2018, 6:05 AM

I general looks good, i think.

Why do the testcases contain nsw attributes?
We transform anyway, and they aren't required, and don't affect the results.
I'd recommend dropping them. Unless there is a purpose?

llvm/test/Transforms/InstSimplify/srem.ll
1 ↗(On Diff #155866)

The first line is missing, it should say it was produced with the tool.

20 ↗(On Diff #155866)

I'd like to see more tests:

define <3 x i32> @negated_operand_vec_undef(<3 x i32> %x) {
  %negx = sub <3 x i32> <i32 0, i32 undef, i32 0>, %x
  %rem = srem <3 x i32> %negx, %x
  ret <3 x i32> %rem
}
define <2 x i32> @negated_operand_vec_nonsplat(<2 x i32> %x) {
  %negx = sub <2 x i32> <i32 0, 1>, %x ; not 0, don't fold
  %rem = srem <2 x i32> %negx, %x
  ret <2 x i32> %rem
}
shchenz updated this revision to Diff 156435.Jul 19 2018, 11:49 PM

fix Roman's comments

lebedev.ri accepted this revision.Jul 20 2018, 1:27 AM

OK, this looks good to me.
Please commit tests first, so the code change only changes the tests, not adds a new ones.

llvm/lib/Analysis/InstructionSimplify.cpp
1112 ↗(On Diff #156435)

s/negatived/negated/? Not sure which is more correct in english.

llvm/test/Transforms/InstSimplify/srem.ll
11 ↗(On Diff #156435)

One more test i didn't think of yet:
https://rise4fun.com/Alive/DplN
isKnownNegation() doesn't care about which operand came from which side.
In this case, it is ok, but in general srem/sdiv is not commutative.
It should be kept in mind, and at least one test should verify that we still fold this case properly.

This revision is now accepted and ready to land.Jul 20 2018, 1:27 AM

more testcases are added at rL337543

llvm/lib/Analysis/InstructionSimplify.cpp
1112 ↗(On Diff #156435)

should be negated.

This revision was automatically updated to reflect the committed changes.