[PATCH] D51398: [InstCombine] Fold (neg (min/max ~X, C)) -> (add (min/max X, ~C), 1)

Sanjay Patel via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Aug 29 06:52:27 PDT 2018


spatel added a subscriber: RKSimon.
spatel added a comment.

https://rise4fun.com/Alive/pAh
Since we know this isn't limited to negate, there should be a 'TODO' about that...or just allow for that possibility in this patch? Not difficult to make the match() account for an arbitrary constant.
Also, whether we use IsFreeToInvert or m_Constant(), I think this fold will work for an arbitrary vector constant, not just a splat. Adding a test for that will make @RKSimon happier. :)



================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1771
 
+  // Fold (neg (min/max ~X, C)) -> (add (min/max X, ~C), 1)
+  if (match(Op0, m_ZeroInt()) && Op1->hasOneUse()) {
----------------
The formula suggests that we have a constant, but the code allows for a variable operand. Either the comment should change to match code (C becomes Y) and add a test for that possibility or change the code to match the comment.

Also, we're inverting min/max, so the result in the formula should read (max/min X, ~C)?


================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1787
+          IsFreeToInvert(LHS, LHS->hasOneUse())) {
+        Value *NotY = Builder.CreateNot(RHS);
+        Value *MinMax = Builder.CreateSelect(
----------------
That should be CreateNot(LHS)? Guessing there's no test coverage for this clause.


================
Comment at: test/Transforms/InstCombine/sub.ll:1061
+; Tests for (neg (max ~X, C)) -> ((min X, ~C) + 1). Same for min.
+define i32 @test64(i32 %x) {
+; CHECK-LABEL: @test64(
----------------
As usual, I'd prefer that the tests are committed first with baseline checks.


https://reviews.llvm.org/D51398





More information about the llvm-commits mailing list