This is an archive of the discontinued LLVM Phabricator instance.

[VectorUtils] Rework the Vector Function Database (VFDatabase).
ClosedPublic

Authored by fpetrogalli on Jan 14 2020, 1:22 PM.

Details

Summary

This commits is a rework of the patch in
https://reviews.llvm.org/D67572.

The rework was requested to prevent out-of-tree performance regression
when vectorizing out-of-tree IR intrinsics. The vectorization of such
intrinsics is enquired via the static function isTLIScalarize. For
detail see the discussion in https://reviews.llvm.org/D67572.

Event Timeline

fpetrogalli created this revision.Jan 14 2020, 1:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 14 2020, 1:22 PM

@uabelho , let me know if the introduction of isTLIScalarize prevents the regressions you have seen in your out-of-tree compiler.

Regards,

Francesco

@uabelho , let me know if the introduction of isTLIScalarize prevents the regressions you have seen in your out-of-tree compiler.

Hi!

I applied this new patch as is and now I don't see the regressions I saw last time.

Nice!

I have removed the LangRef changes. I'll create a separate patch for that.

@uabelho , let me know if the introduction of isTLIScalarize prevents the regressions you have seen in your out-of-tree compiler.

Hi!

I applied this new patch as is and now I don't see the regressions I saw last time.

Great! Happy to hear that. The patch doesn't differ from the original one at https://reviews.llvm.org/D67572, other than for the addition of the isTLIScalarize method and its use in LoopVectorizationLegality.cpp. If you are happy with it, may I ask you to formally approve it so I can submit it? (Feel free to do a full review if you prefer to do so!)

Kind regards,

Francesco

Nice!

fpetrogalli edited the summary of this revision. (Show Details)Jan 15 2020, 9:36 AM
sdesmalen accepted this revision.Jan 15 2020, 9:50 AM

The fix seems sensible to me.

llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp
571

nit: s/scalarcalls/scalar calls/

582

nit: s/emty/empty/

585

nit: unnecessary curly braces.

This revision is now accepted and ready to land.Jan 15 2020, 9:50 AM

If you are happy with it, may I ask you to formally approve it so I can submit it? (Feel free to do a full review if you prefer to do so!)

@sdesmalen beat me to it but it looks good to me too. Thanks!

This revision was automatically updated to reflect the committed changes.
nemanjai added inline comments.
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
3290

This is actually problematic since it does not consider the vectorization factor. As a result, we assume that if we have any function for this call site with any vectorization factor, scalarization is not needed. This is simply not a valid assumption and causes failures on some of our internal benchmarks.

I have posted a patch to address this issue in https://reviews.llvm.org/D74944

The test case that is part of that patch currently asserts after this commit.

fhahn added a comment.Apr 19 2021, 9:02 AM

@fpetrogalli it looks like this patch may be causing https://bugs.llvm.org/show_bug.cgi?id=49950. It would be great if you could take a look.