[Polly] Add vectorize metadata to loops identified as vectorizable by polly #113994

kartcq · 2024-10-29T01:57:18Z

This patch introduces the initial implementation for annotating loops created by Polly. Polly generates RunTimeChecks (RTCs), which result in loop versioning. Specifically, the loop created by Polly is executed when the RTCs pass, otherwise, the original loop is executed.

This patch adds the "llvm.loop.vectorize.enable" metadata, setting it to true for loops created by Polly. It also disables vectorization for the original fallback loop.

This behavior is controlled by the 'polly-annotate-metadata-vectorize' flag, and the annotations are applied only when this flag is enabled. This flag is set to false by default.

NOTE: This commit is initial patch in effort to make polly interact with Loop Vectorizer via metadata.

… polly This patch introduces the initial implementation for annotating loops created by Polly. Polly generates RunTimeChecks (RTCs), which result in loop versioning. Specifically, the loop created by Polly is executed when the RTCs pass, otherwise, the original loop is executed. This patch adds the "llvm.loop.vectorize.enable" metadata, setting it to true for loops created by Polly. It also disables vectorization for the original fallback loop. This behavior is controlled by the 'polly-annotate-metadata-vectorize' flag, and the annotations are applied only when this flag is enabled. This flag is set to false by default. NOTE: This commit is initial patch in effort to make polly interact with Loop Vectorizer via metadata.

github-actions · 2024-10-29T02:00:34Z

✅ With the latest revision this PR passed the C/C++ code formatter.

Meinersbur

Thanks for the PR

Did you check whether LoopVectorize is also going to vectorize the loop without its own RTC check?

polly/include/polly/CodeGen/IRBuilder.h

polly/lib/CodeGen/CodeGeneration.cpp

Co-authored-by: Michael Kruse <[email protected]>

kartcq · 2024-10-30T05:17:45Z

Thanks for the PR

Did you check whether LoopVectorize is also going to vectorize the loop without its own RTC check?

Thanks for reviewing the patch.
For now the answer is no, LoopVectorizer will still create RTCs. We are only preventing the fall back original loop by polly from getting versioned/vectorized by LoopVectorizer. But eventually the goal is to remove such common checks and append only if there are any additional checks from Loop Vectorizer.

Meinersbur

This patch adds the metadata, setting it to true for loops created by Polly.

"llvm.loop.vectorize.enable" set to true will just ignore loop-vectorizer's cost heursitic, and vectorize any loop that is semantically legal to do. Being processed by Polly doesn't mean it becomes automatically profitable to vectorizer, and LoopVectorize would still vectorize it if it thinks it is beneficial without the metadata, which currently also only applies to innermost loops.

Also consider that Clang emits a warning when LoopVectorizer does not apply on a loop with "llvm.loop.vectorize.enable" set to true. Users using Polly would get a lot such warnings.

What are the long-term plans? Atm it is only okay-ish because this behaviour is disabled by a flag.

Meinersbur · 2024-10-30T12:00:33Z

polly/lib/CodeGen/IRBuilder.cpp

 void ScopAnnotator::annotateLoopLatch(BranchInst *B, Loop *L, bool IsParallel,
-                                      bool IsLoopVectorizerDisabled) const {
+                                      bool setVectorizeMetadata,


Suggested change

bool setVectorizeMetadata,

bool SetVectorizeMetadata,

Could add a doxygen comment on what the parameters mean?

I am getting confused with the flag combinations. Since its terrnary logic, consider std::optional<bool> EnableLoopVectorizer where std::nullopt means no metadata set.

Meinersbur · 2024-10-30T12:05:00Z

polly/lib/CodeGen/LoopGenerators.cpp

+      Annotator->annotateLoopLatch(B, NewLoop, Parallel, true,
+                                   !LoopVectDisabled);
+    else if (LoopVectDisabled)
+      Annotator->annotateLoopLatch(B, NewLoop, Parallel, true, false);


Suggested change

Annotator->annotateLoopLatch(B, NewLoop, Parallel, true, false);

Annotator->annotateLoopLatch(B, NewLoop, Parallel, /*SetVectorizeMetadata=*/true, /*EnableLoopVectorizer=*/false);

but why not:

if (Annotator) Annotator->annotateLoopLatch(B, NewLoop, Parallel, /*SetVectorizeMetadata=*/PollyVectorizeMetadata||LoopVectDisabled, /*EnableLoopVectorizer=*/!LoopVectDisabled&&!PollyVectorizeMetadata);

or at least

else Annotator->annotateLoopLatch(B, NewLoop, Parallel, /*SetVectorizeMetadata=*/LoopVectDisabled, /*EnableLoopVectorizer=*/false);

like before this commit

Meinersbur · 2024-10-30T12:14:17Z

polly/lib/CodeGen/LoopGenerators.cpp

-    Annotator->annotateLoopLatch(B, NewLoop, Parallel, LoopVectDisabled);
+
+  // If the 'polly-annotate-metadata-vectorize' flag is passed, we add
+  // the vectorize metadata. Otherwise we fall back to previous behavior


"previous behavior" referes to this commit. That does not make sense if you see this comment only in the source.

Meinersbur · 2024-10-30T12:24:07Z

polly/lib/CodeGen/CodeGeneration.cpp

+  // loop when 'polly-annotate-metadata-vectorize' is passed.
+  if (PollyVectorizeMetadata) {
+    for (Loop *L : LI.getLoopsInPreorder()) {
+      if (S.contains(L)) {


Suggested change

if (S.contains(L)) {

if (!S.contains(L))

continue;

Coding standard

Meinersbur · 2024-10-30T12:25:34Z

polly/lib/CodeGen/CodeGeneration.cpp

+        for (BasicBlock *ControlBB : LoopLatchBlocks) {
+          BranchInst *Br = dyn_cast<BranchInst>(ControlBB->getTerminator());
+          if (Br)
+            Annotator.annotateLoopLatch(Br, L, false, true, false);


Why not directly calling addVectorizeMetadata here? The ScopAnnotator was design with the assumption to be called on generated code only. Using it outside of it may result in unexpected situations.

Co-authored-by: Michael Kruse <[email protected]>

… codegen

Meinersbur · 2024-11-05T12:01:00Z

polly/lib/CodeGen/IRBuilder.cpp

@@ -128,8 +128,28 @@ void ScopAnnotator::popLoop(bool IsParallel) {
  LoopAttrEnv.pop_back();
 }

-void ScopAnnotator::annotateLoopLatch(BranchInst *B, Loop *L, bool IsParallel,
-                                      bool IsLoopVectorizerDisabled) const {
+void ScopAnnotator::addVectorizeMetadata(LLVMContext &Ctx,


Suggested change

void ScopAnnotator::addVectorizeMetadata(LLVMContext &Ctx,

static void addVectorizeMetadata(LLVMContext &Ctx,

AFICS this does not use any ScopAnnotator members

Meinersbur · 2024-11-05T12:03:05Z

polly/lib/CodeGen/CodeGeneration.cpp

+  // for the code flow taken when RTCs fail. Because we don't want the
+  // Loop Vectorizer to come in later and vectorize the original fall back
+  // loop when 'polly-annotate-metadata-vectorize' is passed.
+  if (PollyVectorizeMetadata) {


Suggested change

if (PollyVectorizeMetadata) {

I'd be fine if the loop vectorizer is always disabled for fallback code. Would it means too many test updates?

I am seeing around 19 failures.
Should we have it as separate patch.
Or can it be part of this patch itself ?

@Meinersbur Please provide inputs on if we have to add the test case changes for 19 failures as separate patch.

You can add the test changes into PR. If that's too much work I'd accept the -polly-annotate-metadata-vectorize opt-in as well.

Meinersbur · 2024-11-05T12:04:44Z

polly/lib/CodeGen/IRBuilder.cpp

+// Last argument is optional, if no value is passed, we don't annotate
+// any vectorize metadata.


Interface descriptions usually go to the declaration in the header files as Doxygen comment (/// @param EnableVectorizeMetadata If no value is passed, we don't annotate any vectorize metadata.)

Meinersbur · 2024-11-05T12:26:53Z

polly/lib/CodeGen/LoopGenerators.cpp

+    if (!LoopVectDisabled && !PollyVectorizeMetadata)
+      Annotator->annotateLoopLatch(B, Parallel);
+    else
+      Annotator->annotateLoopLatch(
+          B, Parallel,
+          /*EnableVectorizeMetadata*/ !LoopVectDisabled &&
+              PollyVectorizeMetadata);


Suggested change

if (!LoopVectDisabled && !PollyVectorizeMetadata)

Annotator->annotateLoopLatch(B, Parallel);

else

Annotator->annotateLoopLatch(

B, Parallel,

/*EnableVectorizeMetadata*/ !LoopVectDisabled &&

PollyVectorizeMetadata);

std::optional<bool> EnableVectorizeMetadata;

if (LoopVectDisabled)

EnableVectorizeMetadata = false;

else if (PollyVectorizeMetadata)

EnableVectorizeMetadata = true;

Annotator->annotateLoopLatch(B, Parallel, EnableVectorizeMetadata);

If I got the boolean logic correct

Meinersbur · 2024-11-05T12:40:01Z

polly/lib/CodeGen/CodeGeneration.cpp

+    LLVMContext &Ctx = S.getFunction().getContext();
+    for (Loop *L : LI.getLoopsInPreorder()) {
+      if (!L || !S.contains(L))
+        continue;
+      MDNode *LoopID = L->getLoopID();
+      SmallVector<Metadata *, 3> Args;
+      if (LoopID)
+        for (unsigned i = 0, e = LoopID->getNumOperands(); i != e; ++i)
+          Args.push_back(LoopID->getOperand(i));
+      else
+        Args.push_back(nullptr);
+
+      Annotator.addVectorizeMetadata(Ctx, &Args, false);
+      MDNode *NewLoopID = MDNode::get(Ctx, Args);
+      NewLoopID->replaceOperandWith(0, NewLoopID);
+      L->setLoopID(NewLoopID);
+    }


Suggested change

LLVMContext &Ctx = S.getFunction().getContext();

for (Loop *L : LI.getLoopsInPreorder()) {

if (!L || !S.contains(L))

continue;

MDNode *LoopID = L->getLoopID();

SmallVector<Metadata *, 3> Args;

if (LoopID)

for (unsigned i = 0, e = LoopID->getNumOperands(); i != e; ++i)

Args.push_back(LoopID->getOperand(i));

else

Args.push_back(nullptr);

Annotator.addVectorizeMetadata(Ctx, &Args, false);

MDNode *NewLoopID = MDNode::get(Ctx, Args);

NewLoopID->replaceOperandWith(0, NewLoopID);

L->setLoopID(NewLoopID);

}

#include "llvm/Transforms/Utils/LoopUtils.h"

LLVMContext &Ctx = S.getFunction().getContext();

for (Loop *L : LI.getLoopsInPreorder()) {

if (!S.contains(L))

continue;

addStringMetadataToLoop(L, "llvm.loop.vectorize.enable", 0)

}

A small query here ..
Using addStringMetadataToLoop API makes the value get set as i32.
Something like
!2 = !{!"llvm.loop.vectorize.enable", i32 0}
Though the behavior is same, Is it okay, as the value should be i1 according to LangRef.rst?

It does not make a difference:

llvm-project/llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp

Line 303 in 854cbbf

unsigned Val = C->getZExtValue();

Meinersbur · 2024-11-05T12:41:16Z

polly/lib/CodeGen/IRBuilder.cpp

+  Args->push_back(MDNode::get(Ctx, {PropName, PropValue}));
+}
+
+void addParallelMetadata(LLVMContext &Ctx, SmallVector<Metadata *, 3> *Args,


Suggested change

void addParallelMetadata(LLVMContext &Ctx, SmallVector<Metadata *, 3> *Args,

static void addParallelMetadata(LLVMContext &Ctx, SmallVector<Metadata *, 3> *Args,

kartcq · 2024-11-05T18:48:04Z

This patch adds the metadata, setting it to true for loops created by Polly.

"llvm.loop.vectorize.enable" set to true will just ignore loop-vectorizer's cost heursitic, and vectorize any loop that is semantically legal to do. Being processed by Polly doesn't mean it becomes automatically profitable to vectorizer, and LoopVectorize would still vectorize it if it thinks it is beneficial without the metadata, which currently also only applies to innermost loops.

Also consider that Clang emits a warning when LoopVectorizer does not apply on a loop with "llvm.loop.vectorize.enable" set to true. Users using Polly would get a lot such warnings.

What are the long-term plans? Atm it is only okay-ish because this behaviour is disabled by a flag.

This patch is an initial step in our long-term plan to enhance Polly’s support for the Loop Vectorizer. We are currently evaluating various representative workloads with this patch to identify any gaps, such as instances where Polly is causing the Loop Vectorizer to vectorize unprofitable loops. We are using performance and loop coverage metrics for this evaluation. Ultimately, we aim to implement more constraints on Polly’s side before enabling the metadata that forces vectorization, rather than applying it to all loops created by Polly as we do now.

kartcq · 2024-12-16T10:21:31Z

@Meinersbur @efriedma-quic
Part of this PR is merged in #119188.
For the remaining changes should I modify changes here or create a new PR.
Please provide your suggestions.

efriedma-quic · 2024-12-16T18:40:15Z

I think it's fine to just push the remaining changes here, since the goal here still hasn't changed.

kartcq · 2025-01-02T07:19:37Z

Ping for review @Meinersbur

kartcq · 2025-01-10T05:27:10Z

ping @Meinersbur

Meinersbur

I'd prefer -polly-annotate-metadata-vectorize always-on which requires regression test changes. If you can do that it would be great, but the patch is already fine as-is, so LGTM.

kartcq · 2025-01-22T08:58:35Z

Thanks for the review @Meinersbur. The plan is to enable the flag by default in future.
Merging the patch as is for now.

This patch adds the metadata, setting it to true for loops created by Polly.

"llvm.loop.vectorize.enable" set to true will just ignore loop-vectorizer's cost heursitic, and vectorize any loop that is semantically legal to do. Being processed by Polly doesn't mean it becomes automatically profitable to vectorizer, and LoopVectorize would still vectorize it if it thinks it is beneficial without the metadata, which currently also only applies to innermost loops.
Also consider that Clang emits a warning when LoopVectorizer does not apply on a loop with "llvm.loop.vectorize.enable" set to true. Users using Polly would get a lot such warnings.
What are the long-term plans? Atm it is only okay-ish because this behaviour is disabled by a flag.

This patch is an initial step in our long-term plan to enhance Polly’s support for the Loop Vectorizer. We are currently evaluating various representative workloads with this patch to identify any gaps, such as instances where Polly is causing the Loop Vectorizer to vectorize unprofitable loops. We are using performance and loop coverage metrics for this evaluation. Ultimately, we aim to implement more constraints on Polly’s side before enabling the metadata that forces vectorization, rather than applying it to all loops created by Polly as we do now.

kartcq requested review from Meinersbur and efriedma-quic October 29, 2024 01:58

Update CodeGeneration.cpp

374788e

Meinersbur reviewed Oct 29, 2024

View reviewed changes

polly/include/polly/CodeGen/IRBuilder.h Outdated Show resolved Hide resolved

polly/lib/CodeGen/CodeGeneration.cpp Outdated Show resolved Hide resolved

polly/lib/CodeGen/CodeGeneration.cpp Outdated Show resolved Hide resolved

Apply suggestions from code review

fd61d16

Co-authored-by: Michael Kruse <[email protected]>

Meinersbur reviewed Oct 30, 2024

View reviewed changes

kartcq and others added 3 commits November 5, 2024 02:11

Apply suggestions from code review

075a778

Co-authored-by: Michael Kruse <[email protected]>

Merge branch 'codegen' of https://github.com/kartcq/llvm-project into…

fe01b97

… codegen

Update CodeGeneration.cpp

c368310

Meinersbur reviewed Nov 5, 2024

View reviewed changes

kartcq added 5 commits December 18, 2024 05:07

Remove changes that are already made in llvm#119188

92703e2

Update IRBuilder.cpp

7a51d22

Update basic_vec_annotate.ll

1a07e39

Update IRBuilder.cpp

2e84b67

Merge branch 'main' into codegen

9771c7f

Meinersbur approved these changes Jan 10, 2025

View reviewed changes

kartcq merged commit 76672e3 into llvm:main Jan 22, 2025
8 checks passed

kartcq deleted the codegen branch April 16, 2025 07:37

	Annotator->annotateLoopLatch(B, NewLoop, Parallel, true, false);
	Annotator->annotateLoopLatch(B, NewLoop, Parallel, /SetVectorizeMetadata=/true, /EnableLoopVectorizer=/false);

	void ScopAnnotator::addVectorizeMetadata(LLVMContext &Ctx,
	static void addVectorizeMetadata(LLVMContext &Ctx,

		// Last argument is optional, if no value is passed, we don't annotate
		// any vectorize metadata.

	void addParallelMetadata(LLVMContext &Ctx, SmallVector<Metadata , 3> Args,
	static void addParallelMetadata(LLVMContext &Ctx, SmallVector<Metadata , 3> Args,

[Polly] Add vectorize metadata to loops identified as vectorizable by polly #113994

[Polly] Add vectorize metadata to loops identified as vectorizable by polly #113994

Uh oh!

Conversation

kartcq commented Oct 29, 2024

Uh oh!

github-actions bot commented Oct 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Meinersbur left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kartcq commented Oct 30, 2024

Uh oh!

Meinersbur left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kartcq commented Nov 5, 2024

Uh oh!

kartcq commented Dec 16, 2024

Uh oh!

efriedma-quic commented Dec 16, 2024

Uh oh!

kartcq commented Jan 2, 2025

Uh oh!

kartcq commented Jan 10, 2025

Uh oh!

Meinersbur left a comment

Choose a reason for hiding this comment

Uh oh!

kartcq commented Jan 22, 2025

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Oct 29, 2024 •

edited

Loading