Page MenuHomePhabricator

[LoopVectorizer] give more advice in remark about failure to vectorize call
ClosedPublic

Authored by spatel on Thu, Jan 10, 9:31 AM.

Details

Summary

Something like this is requested by:
https://bugs.llvm.org/show_bug.cgi?id=40265
...and it seems like a common enough case that we should acknowledge it. Not sure if this crosses the line for wordiness in an optimization remark though.

Diff Detail

Repository
rL LLVM

Event Timeline

spatel created this revision.Thu, Jan 10, 9:31 AM

Can we check whether the function could be vectorized if fast math were enabled, so we only show the advice when it's relevant?

"relaxing the floating-point model" is a little confusing... can we explicitly say "consider turning on fast math" or something like that?

spatel updated this revision to Diff 181159.Thu, Jan 10, 2:37 PM

Patch updated:

  1. Try to distinguish a vectorizable libcall from an arbitrary call (I don't see an exact mapping, but "hasOptimizedCodeGen()" looks close).
  2. Add tests to show that we correctly differentiate the 2 cases.
hfinkel added inline comments.Thu, Jan 10, 6:05 PM
lib/Transforms/Vectorize/LoopVectorizationLegality.cpp
726 ↗(On Diff #181159)

I'd prefer it say "fast-math mode" instead of just "fast-math". It would be nice if we could also point users to -fno-math-errno, as that might fix this problem for them and they might not be able to use -ffast-math for the whole translation unit.

Now we already have a problem in the vectorizer because it has a lot of optimization remarks that mention Clang-specific things (flags, pragmas, etc.). The intent of the optimization-remark design was that the frontend callback handler would handle such cases by adding frontend-specific information in the frontend (and not have it embedded here). That didn't happen, and while we should clean this up, in the mean time we might just make the problem incrementally worse and mention flags here too: "try compiling with -fno-math-errno or -ffast-math".

spatel marked an inline comment as done.Fri, Jan 11, 6:12 AM
spatel added inline comments.
lib/Transforms/Vectorize/LoopVectorizationLegality.cpp
726 ↗(On Diff #181159)

Yes, I was trying to avoid clang-specific language here.
And in the motivating bug report, you're exactly right -- we only needed -fno-math-errno to overcome the limitation (that's why I was using the likely too vague "relaxed FP" vocabulary in the previous rev).

spatel updated this revision to Diff 181266.Fri, Jan 11, 7:23 AM

Patch updated:

  1. Added an FP-type constraint to the mathlib check (no point suggesting FP flags if it's not an FP call).
  2. Changed remark text to include clang-specific flags (and suggest/hope that users can translate those to their actual front-end options if this isn't a clang-based invocation).
hfinkel accepted this revision.Fri, Jan 11, 8:38 AM

LGTM

This revision is now accepted and ready to land.Fri, Jan 11, 8:38 AM
Ayal added a comment.Fri, Jan 11, 10:20 AM

This LGTM too, just adding mtcw wondering if these extra checks for more accurate reporting are worth placing under allowExtraAnalysis(); and/or if TLI->isFunctionVectorizable() shouldn't be the one informing the cause of its failure when returning false.

This LGTM too, just adding mtcw wondering if these extra checks for more accurate reporting are worth placing under allowExtraAnalysis(); and/or if TLI->isFunctionVectorizable() shouldn't be the one informing the cause of its failure when returning false.

Those are good questions/comments. I'm not too familiar with the code organization here, but I'll add that to the 'TODO' comment for now, so we don't lose it.

This revision was automatically updated to reflect the committed changes.