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

Allen zhong via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Oct 28 09:23:35 PDT 2022


Allen added inline comments.


================
Comment at: llvm/test/Transforms/InstCombine/add4.ll:170
+  %a0 = add i32 %x, %y
+  %m = mul i32 %x, 4
+  %a1 = add i32 %a0, %m
----------------
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.


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

https://reviews.llvm.org/D136912



More information about the llvm-commits mailing list