Page MenuHomePhabricator

#pragma clang loop vectorize_predicate(enable|disable)
ClosedPublic

Authored by SjoerdMeijer on Jul 15 2019, 7:42 AM.

Details

Summary

This adds a vectorize ''predicate" loop hint, e.g.:

#pragma loop vectorize(enable) vectorize_predicate(enable)

that can be used to indicate to the vectoriser that all statements should be predicated. This allows, for example, 'folding' of the remainder loop into the main loop.

Diff Detail

Repository
rL LLVM

Event Timeline

SjoerdMeijer created this revision.Jul 15 2019, 7:42 AM

IMHO, this loops like an option of a particular transformation, not an independent pragma. E.g.

#pragma clang loop vectorize(enable) vectorize_remainder(predicated)

There could be multiple choices for how to execute remainder iterations, e.g. instead of an epilogue, the first iterations could be executed in an prologue. Or an option where the compiler may assume that the iteration-count is always a multiple of the vector width, etc.

Also consider interactions with other transformations: What would happen with the llvm.loop.tailpredicate metadata after, e.g. loop fusion/distribution? How does the user know whether the pragma had an effect?

Thanks for taking a look and your suggestions!

I noticed your comment here after I replied to the list. As I wrote there, and long story short, I thought I could kill 2 birds with 1 stone, but that doesn't seem to be the case. I agree that for the vectorizer an option like this is much better:

#pragma clang loop vectorize(enable) vectorize_remainder(enable)

I will implement that here because that looks a useful addition that people like, and I think this will also work for me initially.

SjoerdMeijer retitled this revision from Loop #pragma tail_predicate to #pragma clang loop predicate(enable|disable).
SjoerdMeijer edited the summary of this revision. (Show Details)

So I went for:

predicate(enable)

as I think that is most general, best capturing it, but I am of course completely open to bikeshedding names :-)

[serious] Need to update docs.

I'd expect predicate to be an option of another transformation. For vectorization, by convention the #pragma name would be vectorize_predicate and the metadata llvm.loop.vectorize.predicate. As a standalone, it is not clear what it is supposed to do (docs missing) and I am against mixing similar semantics of options of different loop transformations.

I am currently trying to update the semantics of loop transformations such that a transformation order can be defined. An option without transformation it belongs to does not fit well into that. E.g. what happends with llvm.loop.predicate after unrolling/vectorization/distribution/interchange/unroll-and-jam/fusion?

Michael

clang/lib/CodeGen/CGLoopInfo.cpp
426 ↗(On Diff #210314)

[nit] run clang-format over your patch

SjoerdMeijer retitled this revision from #pragma clang loop predicate(enable|disable) to #pragma clang loop vectorize_predicate(enable|disable).
SjoerdMeijer edited the summary of this revision. (Show Details)

Hi Michael, thanks for taking a look again!

Completely agree what you suggested, so I've change the pragma to vectorize_predicate(enable) and the metadata to llvm.loop.vectorize.predicate.

My little plan is as follows:

  • Finish D64916: thanks for reviewing that too!
  • I will follow up on that. I have a local patch that I need to finish that shows how all moving parts work together. I.e., it picks up the metadata, and enables the folding.
  • Finally, I was thinking to follow up with a doc patch. I don't want to advertise this just yet, but want to get everything in, and then add it to the docs.

Does that sound like an idea?

Removed the separate function that created the loop.llvm.vectorize.predicate metadata. This is now just part of function createLoopVectorizeMetadata, that creates all other vectorize metadata.

I prefer having the documentation change to be in the same patch as the functional change. Makes it easier to check whether they match.

clang/test/CodeGenCXX/pragma-loop.cpp
163 ↗(On Diff #210857)

I spent quite some time in the past to ensure that the MDNode matchers use regexes instead of literals. I'd suggest you create a new test file for vectorize_predicate, then you don't have to change anything here.

clang/test/Parser/pragma-loop.cpp
253 ↗(On Diff #210857)

Can you make the #pragma have its own line. You can use expected-error@+1 to make it match another line.

  • Moved the codegen test to a separate file
  • Added a langref description for this new metadata node.
Herald added a project: Restricted Project. · View Herald TranscriptJul 22 2019, 3:19 AM

Is it intentional that this review has no reviewers listed (like, is this a work in progress you don't expect review on yet)?

clang/include/clang/Basic/Attr.td
2985 ↗(On Diff #211042)

You need to update AttrDocs.td for this as well.

SjoerdMeijer added a comment.EditedJul 22 2019, 6:06 AM

Is it intentional that this review has no reviewers listed (like, is this a work in progress you don't expect review on yet)?

No, sorry about this, that's not intentional. It started indeed as a work-in-progress patch when I wrote to the clang/llvm list with an idea about this. I wanted to show some code too, but didn't add reviewers at that point. In the mean time, this patch has evolved quite a bit, but I never bothered to add reviewers, but will do.

You need to update AttrDocs.td for this as well.

Thanks for the suggestion! Will do.

More doc changes added to AttrDocs.td and LangRef.rst

SjoerdMeijer added a comment.EditedJul 23 2019, 2:32 PM

Apologies for the early ping! But I'm off next weeks, so it would be nice to get this in before that if there are no further comments.
Tomorrow, I will upload another diff that builds on top D64916, which enables code-generation with tail loops folded, thus also demonstrating an end to end test.

This revision is now accepted and ready to land.Jul 24 2019, 4:20 AM

Many thanks for reviewing!

This revision was automatically updated to reflect the committed changes.