This will be re-used by the LoadStoreVectorizer
Details
Diff Detail
Event Timeline
Hi Matt,
There's a similar static function in the loop vectorizer. Do you think it would be beneficial to refactor that one to also use the common interface?
I think so. It looks like that one might be buggy since this version seems to be trying to pick the most conservative set of metadata?
As far as I remember, the version in LoopVectorizer propagates metadata from a single instruction, not from a vector of instructions. However, I think it should be just considered as a special case - i.e. we are still propagating metadata from a vector of instructions, but they all are the same. So, if it doesn't complicate the code much, could you please try refactoring it from there too?
Thanks,
Michael
Hi,
Thanks for doing this! Please find some comments inline.
Michael
lib/Analysis/VectorUtils.cpp | ||
---|---|---|
452 | Maybe make VL ArrayRef<Instruction *> - we always cast its elements to Instruction anyway. | |
lib/Transforms/Vectorize/LoopVectorize.cpp | ||
475 | Why drop const? | |
479 | Ditto. | |
676 | Hmm.. propagateMetadata expects ArrayRef<Value *> VL now instead of Instruction *From, doesn't it? Am I missing something? |
lib/Transforms/Vectorize/LoopVectorize.cpp | ||
---|---|---|
676 | ArrayRef has a one-element constructor (strangely enough). http://llvm.org/docs/doxygen/html/classllvm_1_1ArrayRef.html#a3b1f44186f9787d7ffacb54b62d6798c Something does seem to be wrong here, though; the assertion failures I mentioned in http://reviews.llvm.org/D19501#454941 seem to be coming from this patch. Not sure why yet. |
lib/Transforms/Vectorize/LoopVectorize.cpp | ||
---|---|---|
684 | Oh, I bet I see the problem. There's a difference in behavior between this propagateMetadata and the new one. This one ignores unknown metadata -- leaves it in place on "To". The new one explicitly removes it. |
lib/Transforms/Vectorize/LoopVectorize.cpp | ||
---|---|---|
475 | I tried to keep the const, but the MDNode::get*s make this non-const. |
lib/Transforms/Vectorize/LoopVectorize.cpp | ||
---|---|---|
684 | Oh, that's not quite right -- in the event that you pass just one element for "from" to the new propagateMetadata, propagates *everything*. The new propagateMetadata only removes unknown md if there are multiple "from"s. |
This patch seems to fix it for me. https://gist.github.com/7219541341dd9029d0c038ec59246f8b
This is less restrictive than SLPVectorizer's propagateMetadata in that it doesn't touch unknown metadata. But since SLPVectorizer seems to create all its "To" instructions from scratch, that doesn't seem like a material difference.
I'll try to come up with one, but I am not very good with bugpoint, so it may take me a bit.
Many (okay, two) bugs in bugpoint died to bring us this reduced testcase. Turns out it's very close to what I originally tried to write by hand, but subtly different in some way that I don't care to investigate further.
target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128" target triple = "x86_64-unknown-linux-gnu" define void @fn(i8* readonly %first.coerce, i8* readnone %last.coerce, i8* nocapture %result) { for.body.preheader: br label %for.body for.body: %result.addr.05 = phi i8* [ %incdec.ptr, %for.body ], [ %result, %for.body.preheader ] %first.sroa.0.04 = phi i8* [ %incdec.ptr.i.i.i, %for.body ], [ %first.coerce, %for.body.preheader ] %0 = load i8, i8* %first.sroa.0.04, align 1, !range !5 store i8 %0, i8* %result.addr.05, align 1 %incdec.ptr.i.i.i = getelementptr inbounds i8, i8* %first.sroa.0.04, i64 1 %incdec.ptr = getelementptr inbounds i8, i8* %result.addr.05, i64 1 %lnot.i = icmp eq i8* %incdec.ptr.i.i.i, %last.coerce br i1 %lnot.i, label %for.end.loopexit, label %for.body for.end.loopexit: ret void } !5 = !{i8 0, i8 2}
lgtm. Sorry to keep you waiting -- too many patches.
lib/Analysis/VectorUtils.cpp | ||
---|---|---|
459 | clang-format? |
Maybe make VL ArrayRef<Instruction *> - we always cast its elements to Instruction anyway.