[PATCH] D136912: [AArch64] precommit tests for D136623, NFC

[Allen zhong via Phabricator via llvm-commits]

Allen marked 3 inline comments as done.
Allen added inline comments.


================
Comment at: llvm/test/Transforms/InstCombine/add4.ll:169
+;
+  %a0 = add i32 %x, %y
+  %m = mul i32 %x, 4
----------------
Allen wrote:
> spatel wrote:
> > This isn't a different commuted pattern than above. Did you want to commute operands for %a0?
> 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?
Added new case mul_add_common_factor_2_commute to commute operands for %a0


================
Comment at: llvm/test/Transforms/InstCombine/add4.ll:170
+  %a0 = add i32 %x, %y
+  %m = mul i32 %x, 4
+  %a1 = add i32 %a0, %m
----------------
Allen wrote:
> spatel wrote:
> > Allen wrote:
> > > spatel wrote:
> > > > 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. 
> > > ok, I'll delete the case shl_add_common_factor_commute2 with **shl** as it relies other transform.
> > > 
> > 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?
> * 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.
Added both mul and shl tests, thanks.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136912/new/

https://reviews.llvm.org/D136912