[PATCH] D110235: [LoopVectorize] Support reductions that store intermediary result

[Igor Kirillov via Phabricator via llvm-commits]

igor.kirillov marked an inline comment as done.
igor.kirillov added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:453
+  Value *APtr = A->getPointerOperand();
+  Value *BPtr = B->getPointerOperand();
+  if (APtr == BPtr)
----------------
fhahn wrote:
> With opaque pointers, this code may behave differently.
> 
> It's possible to have
> 
> ```
> store i32 0, ptr %x
> store i8 0, ptr %x
> ```
> 
> In that case the pointer operands will be the same, but different store widths. I *think* we should also check that the types of the stored values match for now, as we use this to remove earlier stores. This should only be correct if the later store writes at least as many bits as the earlier stores.
I think if this check is added here, then the purpose of the function would be different (address is still the same even if values have different size). So, instead of that I added a check to a place where those pointers are really processed - see LoopVectorizationLegality.cpp:975. There we now make sure that values stored in an invariant address are of the same type.


================
Comment at: llvm/test/Transforms/LoopVectorize/reduction-with-invariant-store.ll:303
   %iv = phi i64 [ 0, %entry ], [ %iv.next, %for.body ]
-  %sum = phi i32 [ 0, %entry ], [ %sum.1, %for.body ]
+  %sum = phi i32 [ 0, %entry ], [ %sum.2, %for.body ]
   %arrayidx = getelementptr inbounds i32, i32* %src, i64 %iv
----------------
david-arm wrote:
> nit: Looks like an unnecessary change from %sum.1 -> %sum.2.
`sum.1` is `sum + src[i]` and `sum.2` is `sum + src[i] + src[i+1]`. Looks like everything is fine for me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110235