This is an archive of the discontinued LLVM Phabricator instance.

SLPVectorizer: Move propagateMetadata to VectorUtils
ClosedPublic

Authored by arsenm on May 25 2016, 1:00 PM.

Details

Reviewers
jlebar
Summary

This will be re-used by the LoadStoreVectorizer

Diff Detail

Event Timeline

arsenm updated this revision to Diff 58480.May 25 2016, 1:00 PM
arsenm retitled this revision from to SLPVectorizer: Move propagateMetadata to VectorUtils.
arsenm updated this object.
arsenm added a subscriber: llvm-commits.

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?

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

arsenm updated this revision to Diff 59996.Jun 7 2016, 9:58 PM

Use in loop vectorizer

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?

jlebar added a subscriber: jlebar.Jun 10 2016, 1:17 PM
jlebar added inline comments.
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.

jlebar added inline comments.Jun 10 2016, 1:21 PM
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.

arsenm added inline comments.Jun 10 2016, 1:34 PM
lib/Transforms/Vectorize/LoopVectorize.cpp
475

I tried to keep the const, but the MDNode::get*s make this non-const.

jlebar added inline comments.Jun 10 2016, 1:38 PM
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.

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.

Do you have a reduced testcase to go with it?

Do you have a reduced testcase to go with it?

I'll try to come up with one, but I am not very good with bugpoint, so it may take me a bit.

jlebar added a comment.EditedJun 11 2016, 10:54 AM

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}
jlebar requested changes to this revision.Jun 13 2016, 2:11 PM
jlebar added a reviewer: jlebar.

Things seem to work for me with the patch attached above.

This revision now requires changes to proceed.Jun 13 2016, 2:11 PM
arsenm updated this revision to Diff 61043.Jun 16 2016, 3:32 PM
arsenm edited edge metadata.

Merge patch and testcase

jlebar accepted this revision.Jun 30 2016, 10:52 AM
jlebar edited edge metadata.

lgtm. Sorry to keep you waiting -- too many patches.

lib/Analysis/VectorUtils.cpp
459

clang-format?

This revision is now accepted and ready to land.Jun 30 2016, 10:52 AM
arsenm closed this revision.Jun 30 2016, 2:25 PM

r274281