[AArch64] precommit tests for D136623, NFC

[Allen]

Summary

https://reviews.llvm.org/D136623#inline-1319903

[spatel]

llvm/test/Transforms/InstCombine/add4.ll

161

This doesn't match the code. Did you intend to commute the final add operands?

In general, I find it easier to follow the pattern progression if you name the values based on the operation. For example:

%m = mul i32 %x, 5
%a = add i32 %x, 42

[spatel]

We should have tests with extra uses too, so it's clear that the transforms are not creating too many instructions.
Also, change at least one test to use vector types, so we know it works across different types.

[Allen]

a) Add 2 more case, one use + vector
b) rename the values based on the operation

[Allen]

llvm/test/Transforms/InstCombine/add4.ll

161

Yes, at the 1st step, I'll swap the add operands when the mul on the right.

Thanks for your demo case, I rename the values.

[spatel]

llvm/test/Transforms/InstCombine/add4.ll

169

This isn't a different commuted pattern than above. Did you want to commute operands for %a0?

170

I don't understand the motivation to include tests that will be converted to something else (shl) when we also have an explicit test for the shl patterns below here. If we make this mul with some arbitrary constant (for example, 42), then it is clear that we are testing the various commuted pattern possibilities for a real mul (there should be 4 of those?).

If you want to add more tests to verify that shl transforms are working, then write the IR and the test comments in that form to begin, so we are not relying on some other transform to produce the pattern that is tested.

[Allen]

llvm/test/Transforms/InstCombine/add4.ll

169

Yes, the case mul_add_common_factor_commute1 has the operand mul on the operand 1, and
mul_add_common_factor_commute2 has the operand mul on the operand 0.

So I'll swap the operands for mul_add_common_factor_commute1 at the 1st step. Does it make sense ? or I add more detail comment for this tests?

170

ok, I'll delete the case shl_add_common_factor_commute2 with shl as it relies other transform.

[Allen]

a) delete the case shl_add_common_factor_commute2 with shl as it relies other transform
b) rename all the cases and delete the _commute to avoid the misunderstand.

we only the operands when the **mul **operand on the 2nd operand, so in fact it is not commute.

[spatel]

llvm/test/Transforms/InstCombine/add4.ll

170

If the code patch is going to affect patterns with shl, then we must have tests that include shl. Do we need both mul and shl tests?

[Allen]

llvm/test/Transforms/InstCombine/add4.ll

170

• Don't need affect patterns with shl to fix the issue https://github.com/llvm/llvm-project/issues/57255, although it is easy to expand when need, so I delete the test as my change will restrict the Instruction::Mul form to begin.

[spatel]

Please add other reviewers to help review this.

There is some miscommunication happening, and I'm not sure how to fix it:

1. You said that we would not test shl patterns, but the tests have shl (X * 4) --> (X << 2)
2. I asked about commuting %a0 (currently at line 169), but there is still no coverage for %a0 = add i32 %y, %x ?

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp

775 ↗

(On Diff #471575)

The brackets that you are proposing to remove are likely protecting against shadow variable declaration compile-time warnings.

Please remove this diff from this patch.

I suggest one of the following to improve this code (as a separate commit):

1. Rename the set of variables above with unique names (and adjust the code comments to match).
2. Rename the set of variables here with unique names (and adjust the code comments to match).
3. Create a helper function for the Factorization set of folds, so there is no confusion.

[Allen]

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp

775 ↗

(On Diff #471575)

Thanks for suggestion, addressed in https://reviews.llvm.org/D137019

[Allen]

rebase as D137019 and address the remained comment
a) Add commute case mul_add_common_factor_2_commute
b) Add shl tests

[Allen]

llvm/test/Transforms/InstCombine/add4.ll

169

Added new case mul_add_common_factor_2_commute to commute operands for %a0

170

Added both mul and shl tests, thanks.

[spatel]

llvm/test/Transforms/InstCombine/add4.ll

151

As I wrote previously, this is not a good test of "x*4". It is a test of "x<<2" as shown by the CHECKs.
Unit-tests should avoid other transforms, so they do not break or become meaningless from independent changes.

Hopefully, other reviewers can explain this better. Since I can't seem to get this point across, I'll resign.

[Allen]

Delete all the case with "x*4", as it is in fact same to the test of "x<<2", such as mul_add_common_factor_1 and shl_add_common_factor_1

[Allen]

llvm/test/Transforms/InstCombine/add4.ll

151

Sorry for missing addressing

llvm/test/Transforms/InstCombine/add4.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt < %s -passes=instcombine -S | FileCheck %s
 
+declare i32 @use32(i32)
+
 define i64 @match_unsigned(i64 %x) {
 ; CHECK-LABEL: @match_unsigned(
 ; CHECK-NEXT:    [[UREM:%.*]] = urem i64 [[X:%.*]], 19136
 ; CHECK-NEXT:    ret i64 [[UREM]]
 ;
   %t = urem i64 %x, 299
   %t1 = udiv i64 %x, 299
   %t2 = urem i64 %t1, 64

 ;
   %t = urem i32 %x, 299
   %t1 = udiv i32 %x, 299
   %t2 = urem i32 %t1, 147483647
   %t3 = mul i32 %t2, 299
   %t4 = add i32 %t, %t3
   ret i32 %t4
 }
+
+; TODO: (x + (-1)) + (x * 5) --> (x * 6) + (-1)
+define i8 @mul_add_common_factor_0(i8 %x) {
+; CHECK-LABEL: @mul_add_common_factor_0(
+; CHECK-NEXT:    [[A0:%.*]] = add i8 [[X:%.*]], -1
+; CHECK-NEXT:    [[M:%.*]] = mul i8 [[X]], 5
+; CHECK-NEXT:    [[A1:%.*]] = add i8 [[A0]], [[M]]
+; CHECK-NEXT:    ret i8 [[A1]]
+;
+  %a0 = add i8 %x, -1
+  %m = mul i8 %x, 5
+  %a1 = add i8 %a0, %m ; the mul operand is the right operand, should swap the operand
+  ret i8 %a1
+}
+
+; TODO: (x + 2) + (x * t) --> (t + 1) * x + 2
+define i128 @mul_add_common_factor_3(i128 %x, i128 %t) {
+; CHECK-LABEL: @mul_add_common_factor_3(
+; CHECK-NEXT:    [[A0:%.*]] = add i128 [[X:%.*]], 2
+; CHECK-NEXT:    [[M:%.*]] = mul i128 [[X]], [[T:%.*]]
+; CHECK-NEXT:    [[A1:%.*]] = add i128 [[A0]], [[M]]
+; CHECK-NEXT:    ret i128 [[A1]]
+;
+  %a0 = add i128 %x, 2
+  %m = mul i128 %x, %t
+  %a1 = add i128 %a0, %m
+  ret i128 %a1
+}
+
+; TODO: (x << 2) + (x - 1) --> (x * 5) + (-1)
+define i16 @shl_add_common_factor_1(i16 %x) {
+; CHECK-LABEL: @shl_add_common_factor_1(
+; CHECK-NEXT:    [[A0:%.*]] = add i16 [[X:%.*]], -1
+; CHECK-NEXT:    [[S:%.*]] = shl i16 [[X]], 2
+; CHECK-NEXT:    [[A1:%.*]] = add i16 [[S]], [[A0]]
+; CHECK-NEXT:    ret i16 [[A1]]
+;
+  %a0 = add i16 %x, -1
+  %s = shl i16 %x, 2
+  %a1 = add i16 %s, %a0 ; the shl operand is the left operand
+  ret i16 %a1
+}
+
+; TODO: (x + y) + (x << 2) --> (x * 5) + y
+define i32 @shl_add_common_factor_2(i32 %x, i32 %y) {
+; CHECK-LABEL: @shl_add_common_factor_2(
+; CHECK-NEXT:    [[A0:%.*]] = add i32 [[X:%.*]], [[Y:%.*]]
+; CHECK-NEXT:    [[S:%.*]] = shl i32 [[X]], 2
+; CHECK-NEXT:    [[A1:%.*]] = add i32 [[A0]], [[S]]
+; CHECK-NEXT:    ret i32 [[A1]]
+;
+  %a0 = add i32 %x, %y
+  %s = shl i32 %x, 2
+  %a1 = add i32 %a0, %s ; the shl operand is the right operand
+  ret i32 %a1
+}
+
+; TODO: (y + x) + (x << 2) --> (x * 5) + y
+define i32 @shl_add_common_factor_2_commute(i32 %x, i32 %y) {
+; CHECK-LABEL: @shl_add_common_factor_2_commute(
+; CHECK-NEXT:    [[A0:%.*]] = add i32 [[Y:%.*]], [[X:%.*]]
+; CHECK-NEXT:    [[S:%.*]] = shl i32 [[X]], 2
+; CHECK-NEXT:    [[A1:%.*]] = add i32 [[A0]], [[S]]
+; CHECK-NEXT:    ret i32 [[A1]]
+;
+  %a0 = add i32 %y, %x ; swap the operand
+  %s = shl i32 %x, 2
+  %a1 = add i32 %a0, %s
+  ret i32 %a1
+}
+
+; Negative test: The transformation should not create more instructions
+define i32 @shl_add_common_factor_4(i32 %x) {
+; CHECK-LABEL: @shl_add_common_factor_4(
+; CHECK-NEXT:    [[A0:%.*]] = add i32 [[X:%.*]], -1
+; CHECK-NEXT:    [[S:%.*]] = shl i32 [[X]], 2
+; CHECK-NEXT:    call void @use32(i32 [[S]])
+; CHECK-NEXT:    [[A1:%.*]] = add i32 [[S]], [[A0]]
+; CHECK-NEXT:    ret i32 [[A1]]
+;
+  %a0 = add i32 %x, -1
+  %s = shl i32 %x, 2
+  call void @use32(i32 %s) ; an extra use
+  %a1 = add i32 %s, %a0
+  ret i32 %a1
+}
+
+; TODO: (x << 2) + (x + 3) --> (x * 5) + 3
+define <2 x i64> @shl_add_common_factor_5(<2 x i64> %x) {
+; CHECK-LABEL: @shl_add_common_factor_5(
+; CHECK-NEXT:    [[A0:%.*]] = add <2 x i64> [[X:%.*]], <i64 3, i64 3>
+; CHECK-NEXT:    [[S:%.*]] = shl <2 x i64> [[X]], <i64 2, i64 2>
+; CHECK-NEXT:    [[A1:%.*]] = add <2 x i64> [[S]], [[A0]]
+; CHECK-NEXT:    ret <2 x i64> [[A1]]
+;
+  %a0 = add <2 x i64> %x, <i64 3, i64 3>
+  %s = shl <2 x i64> %x, <i64 2, i64 2> ; vector type
+  %a1 = add <2 x i64> %s, %a0
+  ret <2 x i64> %a1
+}