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

[Allen zhong via Phabricator via llvm-commits]

Allen marked an inline comment 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
----------------
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?


================
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:
> 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.



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

https://reviews.llvm.org/D136912