[LV] Check isPredInst instead of isScalarWithPred in uniform analysis. #98892

fhahn · 2024-07-15T12:52:55Z

Any instruction marked as uniform will result in a uniform VPReplicateRecipe. If it requires predication, it will be placed in a replicate region, even if isScalarWithPredication returns false.

Check isPredicatedInst instead of isScalarWithPredication to avoid generating uniform VPReplicateRecipes placed inside a replicate region. This fixes an assertion when using scalable VFs.

Fixes #80416.
Fixes #94328.
Fixes #99625.

llvmbot · 2024-07-15T12:53:32Z

@llvm/pr-subscribers-llvm-transforms

Author: Florian Hahn (fhahn)

Changes

Any instruction marked as uniform will result in a uniform VPReplicateRecipe. If it requires predication, it will be placed in a replicate region, even if isScalarWithPredication returns false.

Check isPredicatedInst instead of isScalarWithPredication to avoid generating uniform VPReplicateRecipes placed inside a replicate region. This fixes an assertion when using scalable VFs.

Fixes #80416. Fixes #94328.

Full diff: https://github.com/llvm/llvm-project/pull/98892.diff

2 Files Affected:

(modified) llvm/lib/Transforms/Vectorize/LoopVectorize.cpp (+5-4)
(added) llvm/test/Transforms/LoopVectorize/AArch64/divs-with-scalable-vfs.ll (+92)

diff --git a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
index 5520baef7152d..296a346fa7ae1 100644
--- a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
+++ b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
@@ -3907,7 +3907,7 @@ void LoopVectorizationCostModel::collectLoopUniforms(ElementCount VF) {
   SetVector<Instruction *> Worklist;
 
   // Add uniform instructions demanding lane 0 to the worklist. Instructions
-  // that are scalar with predication must not be considered uniform after
+  // that are require predication must not be considered uniform after
   // vectorization, because that would create an erroneous replicating region
   // where only a single instance out of VF should be formed.
   // TODO: optimize such seldom cases if found important, see PR40816.
@@ -3917,9 +3917,10 @@ void LoopVectorizationCostModel::collectLoopUniforms(ElementCount VF) {
                         << *I << "\n");
       return;
     }
-    if (isScalarWithPredication(I, VF)) {
-      LLVM_DEBUG(dbgs() << "LV: Found not uniform being ScalarWithPredication: "
-                        << *I << "\n");
+    if (isPredicatedInst(I)) {
+      LLVM_DEBUG(
+          dbgs() << "LV: Found not uniform due to requiring predication: " << *I
+                 << "\n");
       return;
     }
     LLVM_DEBUG(dbgs() << "LV: Found uniform instruction: " << *I << "\n");
diff --git a/llvm/test/Transforms/LoopVectorize/AArch64/divs-with-scalable-vfs.ll b/llvm/test/Transforms/LoopVectorize/AArch64/divs-with-scalable-vfs.ll
new file mode 100644
index 0000000000000..6fd74d1416873
--- /dev/null
+++ b/llvm/test/Transforms/LoopVectorize/AArch64/divs-with-scalable-vfs.ll
@@ -0,0 +1,92 @@
+; RUN: opt -p loop-vectorize -mtriple aarch64 -mcpu=neoverse-v1 -S %s | FileCheck %s
+
+; Test case for https://github.com/llvm/llvm-project/issues/94328.
+define void @sdiv_feeding_gep(ptr %dst, i32 %x, i64 %M, i64 %conv6, i64 %N) {
+entry:
+  %conv61 = zext i32 %x to i64
+  br label %loop
+
+loop:
+  %iv = phi i64 [ 0, %entry ], [ %iv.next, %loop ]
+  %div18 = sdiv i64 %M, %conv6
+  %conv20 = trunc i64 %div18 to i32
+  %mul30 = mul i64 %div18, %conv61
+  %sub31 = sub i64 %iv, %mul30
+  %conv34 = trunc i64 %sub31 to i32
+  %mul35 = mul i32 %x, %conv20
+  %add36 = add i32 %mul35, %conv34
+  %idxprom = sext i32 %add36 to i64
+  %gep = getelementptr double, ptr %dst, i64 %idxprom
+  store double 0.000000e+00, ptr %gep, align 8
+  %iv.next = add i64 %iv, 1
+  %ec = icmp eq i64 %iv.next, %N
+  br i1 %ec, label %exit, label %loop
+
+exit:
+  ret void
+}
+
+define void @sdiv_feeding_gep_predicated(ptr %dst, i32 %x, i64 %M, i64 %conv6, i64 %N) {
+entry:
+  %conv61 = zext i32 %x to i64
+  br label %loop
+
+loop:
+  %iv = phi i64 [ 0, %entry ], [ %iv.next, %loop.latch ]
+  %c = icmp ule i64 %iv, %M
+  br i1 %c, label %then, label %loop.latch
+
+then:
+  %div18 = sdiv i64 %M, %conv6
+  %conv20 = trunc i64 %div18 to i32
+  %mul30 = mul i64 %div18, %conv61
+  %sub31 = sub i64 %iv, %mul30
+  %conv34 = trunc i64 %sub31 to i32
+  %mul35 = mul i32 %x, %conv20
+  %add36 = add i32 %mul35, %conv34
+  %idxprom = sext i32 %add36 to i64
+  %gep = getelementptr double, ptr %dst, i64 %idxprom
+  store double 0.000000e+00, ptr %gep, align 8
+  br label %loop.latch
+
+loop.latch:
+  %iv.next = add i64 %iv, 1
+  %ec = icmp eq i64 %iv.next, %N
+  br i1 %ec, label %exit, label %loop
+
+exit:
+  ret void
+}
+
+; Test case for https://github.com/llvm/llvm-project/issues/80416.
+define void @udiv_urem_feeding_gep(i64 %x, ptr %dst, i64 %N) {
+entry:
+  %mul.1.i = mul i64 %x, %x
+  %mul.2.i = mul i64 %mul.1.i, %x
+  br label %loop
+
+loop:
+  %iv = phi i64 [ 0, %entry ], [ %iv.next, %loop ]
+  %div.i = udiv i64 %iv, %mul.2.i
+  %rem.i = urem i64 %iv, %mul.2.i
+  %div.1.i = udiv i64 %rem.i, %mul.1.i
+  %rem.1.i = urem i64 %rem.i, %mul.1.i
+  %div.2.i = udiv i64 %rem.1.i, %x
+  %rem.2.i = urem i64 %rem.1.i, %x
+  %mul.i = mul i64 %x, %div.i
+  %add.i = add i64 %mul.i, %div.1.i
+  %mul.1.i9 = mul i64 %add.i, %x
+  %add.1.i = add i64 %mul.1.i9, %div.2.i
+  %mul.2.i11 = mul i64 %add.1.i, %x
+  %add.2.i = add i64 %mul.2.i11, %rem.2.i
+  %sext.i = shl i64 %add.2.i, 32
+  %conv6.i = ashr i64 %sext.i, 32
+  %gep = getelementptr i64, ptr %dst, i64 %conv6.i
+  store i64 %div.i, ptr %gep, align 4
+  %iv.next = add i64 %iv, 1
+  %exitcond.not = icmp eq i64 %iv, %N
+  br i1 %exitcond.not, label %exit, label %loop
+
+exit:
+  ret void
+}

github-actions · 2024-07-15T13:35:54Z

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

Any instruction marked as uniform will result in a uniform VPReplicateRecipe. If it requires predication, it will be placed in a replicate region, even if isScalarWithPredication returns false. Check isPredicatedInst instead of isScalarWithPredication to avoid generating uniform VPReplicateRecipes placed inside a replicate region. This fixes an assertion when using scalable VFs. Fixes llvm#80416. Fixes llvm#94328.

alexey-bataev

LG with a nit

alexey-bataev · 2024-07-15T13:41:48Z

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

@@ -3907,7 +3907,7 @@ void LoopVectorizationCostModel::collectLoopUniforms(ElementCount VF) {
  SetVector<Instruction *> Worklist;

  // Add uniform instructions demanding lane 0 to the worklist. Instructions
-  // that are scalar with predication must not be considered uniform after
+  // that are require predication must not be considered uniform after


Suggested change

// that are require predication must not be considered uniform after

// that require predication must not be considered uniform after

Thanks, fixed!

When folding the tail, at least one of the lanes must execute unconditionally. If the divisor is loop-invariant no predication is needed, as predication would not prevent the divide-by-0 on the executed lane. Depends on llvm#98892.

ayalz

Does this suggest that isScalarWithPredication misses some candidates, namely isPredicatedInst()'s considered - and now turned down - by addToWorklistIfAllowed()? There may be a phase ordering issue in how scalars, uniforms, and predicated instructions are classified.

Adding some thoughts inline.

ayalz · 2024-07-15T21:42:35Z

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

@@ -9482,6 +9483,8 @@ void VPInterleaveRecipe::execute(VPTransformState &State) {
 void VPReplicateRecipe::execute(VPTransformState &State) {
  Instruction *UI = getUnderlyingInstr();
  if (State.Instance) { // Generate a single instance.
+    assert((State.VF.isScalar() || !isUniform()) &&
+           "uniform recipe shouldn't be predicated");


Could this assert be committed separately?

Better catch this earlier, say assert (!IsUniform || !IsPredicated) upon replicate recipe construction and/or at handleReplication()?

Note the if (IsUniform) below - would be good to be consistent.

Added in 3717776 in handleReplication. There is some additional code there that converts predicated assumes to uniform for scalable vectors which may should be revisited. (Predicated assumes are later removed by a VPlan-to-VPlan transform)

ayalz · 2024-07-15T21:44:36Z

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

@@ -3907,7 +3907,7 @@ void LoopVectorizationCostModel::collectLoopUniforms(ElementCount VF) {
  SetVector<Instruction *> Worklist;

  // Add uniform instructions demanding lane 0 to the worklist. Instructions
-  // that are scalar with predication must not be considered uniform after
+  // that are require predication must not be considered uniform after
  // vectorization, because that would create an erroneous replicating region
  // where only a single instance out of VF should be formed.
  // TODO: optimize such seldom cases if found important, see PR40816.


Do the github issues offer additional seldom cases of lost uniformity potentially worth optimizing if important, to be added to TODO?

After reading through it (On GH #40162), I don't think so. AFAICT there is some potential improvements by creating interleave groups (or at least their runtime guards) later. Remove the TODO here

ayalz · 2024-07-15T21:52:06Z

llvm/test/Transforms/LoopVectorize/AArch64/divs-with-scalable-vfs.ll

@@ -0,0 +1,391 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5


Worth pre-committing these tests to first record their current xfail behavior?

Done as 270f5e4

ayalz · 2024-07-15T21:54:30Z

llvm/test/Transforms/LoopVectorize/AArch64/divs-with-scalable-vfs.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
+; RUN: opt -p loop-vectorize -mtriple aarch64 -mcpu=neoverse-v1 -S %s | FileCheck %s
+
+; Test case for https://github.com/llvm/llvm-project/issues/94328.


Wonder if a smaller reproducer could be devised?

Unfortunately I wasn't able to

Add assertion ensuring invariant on construction, split off as suggested from #98892.

Currently the tests crash, due to a VPReplicateRecipe getting predicated for scalable vectors. Precommits tests for #98892. Test cases for * #80416 and * #94328

…98904) When folding the tail, at least one of the lanes must execute unconditionally. If the divisor is loop-invariant no predication is needed, as predication would not prevent the divide-by-0 on the executed lane. Depends on #98892. PR: #98904

Summary: Add assertion ensuring invariant on construction, split off as suggested from #98892. Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60250783

Summary: Currently the tests crash, due to a VPReplicateRecipe getting predicated for scalable vectors. Precommits tests for #98892. Test cases for * #80416 and * #94328 Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60251705

#98892) Summary: Any instruction marked as uniform will result in a uniform VPReplicateRecipe. If it requires predication, it will be placed in a replicate region, even if isScalarWithPredication returns false. Check isPredicatedInst instead of isScalarWithPredication to avoid generating uniform VPReplicateRecipes placed inside a replicate region. This fixes an assertion when using scalable VFs. Fixes #80416. Fixes #94328. Fixes #99625. PR: #98892 Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60251530

…98904) Summary: When folding the tail, at least one of the lanes must execute unconditionally. If the divisor is loop-invariant no predication is needed, as predication would not prevent the divide-by-0 on the executed lane. Depends on #98892. PR: #98904 Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60250648

fhahn requested review from rengolin, davemgreen, ayalz and aniragil July 15, 2024 12:52

llvmbot added vectorizers llvm:transforms labels Jul 15, 2024

fhahn force-pushed the lv-uniform-predinst branch from 369b5ad to b19facf Compare July 15, 2024 13:30

fhahn force-pushed the lv-uniform-predinst branch from b19facf to b6ab904 Compare July 15, 2024 13:39

alexey-bataev approved these changes Jul 15, 2024

View reviewed changes

fhahn mentioned this pull request Jul 15, 2024

[LV] Don't predicate divs with invariant divisor when folding tail #98904

Merged

ayalz reviewed Jul 15, 2024

View reviewed changes

fhahn added a commit that referenced this pull request Jul 18, 2024

[LV] Assert uniform recipes don't get predicated for when vectorizing.

3717776

Add assertion ensuring invariant on construction, split off as suggested from #98892.

fhahn added 3 commits July 18, 2024 21:59

Merge remote-tracking branch 'origin/main' into lv-uniform-predinst

2a40a3c

Merge remote-tracking branch 'origin/main' into lv-uniform-predinst

def9add

!fixup address comments, thanks!

e3293bb

Merge remote-tracking branch 'origin/main' into lv-uniform-predinst

2afd499

fhahn mentioned this pull request Jul 19, 2024

Assertion fail "Should not predicate a uniform recipe" #99625

Closed

fhahn merged commit 008df3c into llvm:main Jul 19, 2024
5 of 7 checks passed

fhahn deleted the lv-uniform-predinst branch July 19, 2024 11:02

eastB233 mentioned this pull request Jul 23, 2024

[VPlan] Treat Div/Rem as not Predicated if divisor is invariant #95191

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[LV] Check isPredInst instead of isScalarWithPred in uniform analysis. #98892

[LV] Check isPredInst instead of isScalarWithPred in uniform analysis. #98892

Uh oh!

fhahn commented Jul 15, 2024 •

edited

Loading

Uh oh!

llvmbot commented Jul 15, 2024

Uh oh!

github-actions bot commented Jul 15, 2024 •

edited

Loading

Uh oh!

alexey-bataev left a comment

Uh oh!

alexey-bataev Jul 15, 2024

Uh oh!

fhahn Jul 19, 2024

Uh oh!

ayalz left a comment

Uh oh!

ayalz Jul 15, 2024

Uh oh!

fhahn Jul 19, 2024

Uh oh!

ayalz Jul 15, 2024

Uh oh!

fhahn Jul 19, 2024

Uh oh!

ayalz Jul 15, 2024

Uh oh!

fhahn Jul 19, 2024

Uh oh!

ayalz Jul 15, 2024

Uh oh!

fhahn Jul 19, 2024

Uh oh!

Uh oh!

Uh oh!

	// that are require predication must not be considered uniform after
	// that require predication must not be considered uniform after

		@@ -0,0 +1,391 @@
		; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5

[LV] Check isPredInst instead of isScalarWithPred in uniform analysis. #98892

[LV] Check isPredInst instead of isScalarWithPred in uniform analysis. #98892

Uh oh!

Conversation

fhahn commented Jul 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Jul 15, 2024

Uh oh!

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

Uh oh!

Uh oh!

alexey-bataev 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!

ayalz 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!

Uh oh!

Uh oh!

fhahn commented Jul 15, 2024 •

edited

Loading

github-actions bot commented Jul 15, 2024 •

edited

Loading