[PATCH] D42032: [LLVM][PASSES][InstCombine] Fix (a + a + ...) / a cases

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

This looks right, but see inline comments for some small improvements. Do you have commit access?

Note: we're missing the related 'rem' transforms for these patterns...but I think they would go back in instsimplify:
https://rise4fun.com/Alive/wqHz



================
Comment at: lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1009
+  // (X << Y) / X -> 1 << Y
+  Value *Y = nullptr;
+  Constant *C = ConstantInt::get(Op1->getType(), 1);
----------------
No need to initialize this; we don't touch 'Y' if the matches fail.
Feel free to fix the similar line above here as a clean-up commit.


================
Comment at: lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1010
+  Value *Y = nullptr;
+  Constant *C = ConstantInt::get(Op1->getType(), 1);
+  if (IsSigned && match(Op0, m_NSWShl(m_Specific(Op1), m_Value(Y))))
----------------
Probably best not to speculatively create this constant. I think you can move this into the 'Create' lines below without violating the 80-column limit. :)


================
Comment at: test/Transforms/InstCombine/div-shift.ll:117-119
+; CHECK:         %shl = shl i32 %x, 2
+; CHECK-NEXT:    %r = sdiv i32 %shl, %x
+; CHECK-NEXT:    ret i32 %r
----------------
I always suggest using the script at utils/update_test_checks.py to auto-generate the check lines. The regex that it would use here are more resilient to underlying cosmetic changes in instcombine (someone might want to give the values special names, and then this test would fail to match).

I also prefer to check all of the tests in first to show the current output of trunk. That way if your code commit is reverted for some reason, at least the tests will survive and show what was lost by the revert.


https://reviews.llvm.org/D42032