[PATCH] D13996: Fix SLPVectorizer commutativity reordering

[Michael Zolotukhin via llvm-commits]

mzolotukhin added a comment.

Hi Mehdi,

As before, the code looks good to me, some comments on the test are inline.

Thanks,
Michael


================
Comment at: test/Transforms/SLPVectorizer/X86/commutativity.ll:7-8
@@ +6,4 @@
+
+; CHECK:  store <16 x i8>
+; CHECK-NOT:  store
+
----------------
Isn't the second check (CHECK-NOT) redundant here? How is it possible, that we have `store <16 x i8>` and one more store after it?

================
Comment at: test/Transforms/SLPVectorizer/X86/commutativity.ll:17-18
@@ +16,4 @@
+
+; Function Attrs: nounwind ssp uwtable
+define void @foo(i8 %a, i8 %b, i8 %c) #0 {
+.entry:
----------------
As far as I understood, the test checks if we can reorder operands to create a broadcasted operand. Do we need (or do we have already) a separate test, that will check that we try to commute to get 'same opcode' operands?

Like:
```
%mul1 = mul ...
%mul2 = mul ...
%sub = sub ...
%div = div ...

%x = add %sub, %mul1  ; <<< Need to reorder commutative operands to get
%y = add %mul2, %div  ; <<< MUL at the same position for %x and %y

store %x
store %y
```


http://reviews.llvm.org/D13996