[mlir][affine] Add Check for 'affine.for' Bound Map Results #127105

ayokunle321 · 2025-02-13T18:39:31Z

Fixes issue #120001.

I learnt that we shouldn’t assert on IR that can be constructed and passes the verifier. In this case, it appears that the verifier is insufficiently strict and should be tightened. Since the maps used in the affine.for op do not have results, I believe the IR verification should fail however it does not.

Minimal example:

#map = affine_map<() -> ()>
module {
func.func @main() {
affine.for %arg0 = max #map() to min #map() {
}
return
}
}

This gets past the verifier, but I added a check to see that it causes an error if an affine.for has a bound map with no result.

@krzysz00 @matthias-springer @kazutakahirata @MaheshRavishankar Open to any comments or clarifications :)

llvmbot · 2025-02-13T18:40:07Z

@llvm/pr-subscribers-mlir

Author: Ayokunle Amodu (ayokunle321)

Changes

Related to issue #120001.

I learnt that we shouldn’t assert on IR that can be constructed and passes the verifier. In this case, it appears that the verifier is insufficiently strict and should be tightened. Since the maps used in the affine.for op below do not have results, the IR verification should fail however it does not.

Minimal example:

#map = affine_map<() -> ()>
module {
func.func @main() {
affine.for %arg0 = max #map() to min #map() {
}
return
}
}

This gets past the verifier, but I added a check to see that it causes an error if an affine.for has a bound map with no result.

Open to any comments or clarifications :)

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

2 Files Affected:

(modified) mlir/lib/Dialect/Affine/IR/AffineOps.cpp (+6)
(modified) mlir/test/Dialect/Affine/invalid.mlir (+22)

diff --git a/mlir/lib/Dialect/Affine/IR/AffineOps.cpp b/mlir/lib/Dialect/Affine/IR/AffineOps.cpp
index 147f5dd7a24b6..06e26e887b050 100644
--- a/mlir/lib/Dialect/Affine/IR/AffineOps.cpp
+++ b/mlir/lib/Dialect/Affine/IR/AffineOps.cpp
@@ -1903,6 +1903,12 @@ LogicalResult AffineForOp::verifyRegions() {
                                              getUpperBoundMap().getNumDims())))
       return failure();
 
+  if (getLowerBoundMap().getNumResults() < 1)
+    return emitOpError("expected lower bound map to have at least one result");
+
+  if (getUpperBoundMap().getNumResults() < 1)
+    return emitOpError("expected upper bound map to have at least one result");
+    
   unsigned opNumResults = getNumResults();
   if (opNumResults == 0)
     return success();
diff --git a/mlir/test/Dialect/Affine/invalid.mlir b/mlir/test/Dialect/Affine/invalid.mlir
index 44e484b9ba598..5a3243cb074c1 100644
--- a/mlir/test/Dialect/Affine/invalid.mlir
+++ b/mlir/test/Dialect/Affine/invalid.mlir
@@ -523,3 +523,25 @@ func.func @dynamic_dimension_index() {
   }) : () -> ()
   return
 }
+
+// -----
+
+#map = affine_map<() -> ()>
+#map1 = affine_map<() -> (1)>
+func.func @no_lower_bound() {
+  // expected-error@+1 {{'affine.for' op expected lower bound map to have at least one result}}
+  affine.for %i = max #map() to min #map1() {
+  }
+  return
+}
+
+// -----
+
+#map = affine_map<() -> ()>
+#map1 = affine_map<() -> (1)>
+func.func @no_upper_bound() {
+  // expected-error@+1 {{'affine.for' op expected upper bound map to have at least one result}}
+  affine.for %i = max #map1() to min #map() {
+  }
+  return
+}

llvmbot · 2025-02-13T18:40:07Z

@llvm/pr-subscribers-mlir-affine

Author: Ayokunle Amodu (ayokunle321)

Changes

Related to issue #120001.

I learnt that we shouldn’t assert on IR that can be constructed and passes the verifier. In this case, it appears that the verifier is insufficiently strict and should be tightened. Since the maps used in the affine.for op below do not have results, the IR verification should fail however it does not.

Minimal example:

#map = affine_map<() -> ()>
module {
func.func @main() {
affine.for %arg0 = max #map() to min #map() {
}
return
}
}

This gets past the verifier, but I added a check to see that it causes an error if an affine.for has a bound map with no result.

Open to any comments or clarifications :)

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

2 Files Affected:

(modified) mlir/lib/Dialect/Affine/IR/AffineOps.cpp (+6)
(modified) mlir/test/Dialect/Affine/invalid.mlir (+22)

diff --git a/mlir/lib/Dialect/Affine/IR/AffineOps.cpp b/mlir/lib/Dialect/Affine/IR/AffineOps.cpp
index 147f5dd7a24b6..06e26e887b050 100644
--- a/mlir/lib/Dialect/Affine/IR/AffineOps.cpp
+++ b/mlir/lib/Dialect/Affine/IR/AffineOps.cpp
@@ -1903,6 +1903,12 @@ LogicalResult AffineForOp::verifyRegions() {
                                              getUpperBoundMap().getNumDims())))
       return failure();
 
+  if (getLowerBoundMap().getNumResults() < 1)
+    return emitOpError("expected lower bound map to have at least one result");
+
+  if (getUpperBoundMap().getNumResults() < 1)
+    return emitOpError("expected upper bound map to have at least one result");
+    
   unsigned opNumResults = getNumResults();
   if (opNumResults == 0)
     return success();
diff --git a/mlir/test/Dialect/Affine/invalid.mlir b/mlir/test/Dialect/Affine/invalid.mlir
index 44e484b9ba598..5a3243cb074c1 100644
--- a/mlir/test/Dialect/Affine/invalid.mlir
+++ b/mlir/test/Dialect/Affine/invalid.mlir
@@ -523,3 +523,25 @@ func.func @dynamic_dimension_index() {
   }) : () -> ()
   return
 }
+
+// -----
+
+#map = affine_map<() -> ()>
+#map1 = affine_map<() -> (1)>
+func.func @no_lower_bound() {
+  // expected-error@+1 {{'affine.for' op expected lower bound map to have at least one result}}
+  affine.for %i = max #map() to min #map1() {
+  }
+  return
+}
+
+// -----
+
+#map = affine_map<() -> ()>
+#map1 = affine_map<() -> (1)>
+func.func @no_upper_bound() {
+  // expected-error@+1 {{'affine.for' op expected upper bound map to have at least one result}}
+  affine.for %i = max #map1() to min #map() {
+  }
+  return
+}

github-actions · 2025-02-13T18:41:17Z

⚠️ We detected that you are using a GitHub private e-mail address to contribute to the repo.
Please turn off Keep my email addresses private setting in your account.
See LLVM Discourse for more information.

github-actions · 2025-02-13T18:42:51Z

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

krzysz00

Changes approved, but you don't need to comment what the code does

mlir/lib/Dialect/Affine/IR/AffineOps.cpp

bondhugula · 2025-02-18T10:05:30Z

Please rebase, retest, and merge.

bondhugula · 2025-02-18T10:37:41Z

Looks like this has exposed another issue/bug to fix. I can take that over if you prefer.

ayokunle321 · 2025-02-18T17:36:31Z

@bondhugula sure you can take over. So I should not worry about rebasing and retesting?

bondhugula · 2025-02-19T10:27:39Z

@bondhugula sure you can take over. So I should not worry about rebasing and retesting?

I'll debug the issue exposed, fix it and then we'll merge this.

bondhugula · 2025-02-19T15:22:07Z

@bondhugula sure you can take over. So I should not worry about rebasing and retesting?

The failing test case is one where an invalid IR was being generated unchecked to start with. It's now exposed with this PR.

llvm-project/mlir/test/Dialect/Affine/affine-data-copy.mlir

Line 292 in 0b63dfb

// CHECK-LABEL: func @affine_parallel

Created this issue for it: #127808
I'll post the fix and you can merge this post rebase.

bondhugula · 2025-02-19T15:29:43Z

@bondhugula sure you can take over. So I should not worry about rebasing and retesting?

The failing test case is one where an invalid IR was being generated unchecked to start with. It's now exposed with this PR.

llvm-project/mlir/test/Dialect/Affine/affine-data-copy.mlir

Line 292 in 0b63dfb

// CHECK-LABEL: func @affine_parallel

Created this issue for it: #127808 I'll post the fix and you can merge this post rebase.

Fixed by #127809

ayokunle321 · 2025-02-19T20:40:45Z

@bondhugula I did was able to successfully rebase on my local without conflicts:

however GitHub's showing this:

Any suggestions?

CoTinker · 2025-02-20T01:55:12Z

mlir/lib/Dialect/Affine/Utils/LoopUtils.cpp

@@ -2001,8 +2001,15 @@ static LogicalResult generateCopy(
  }

  SmallVector<AffineMap, 4> lbMaps(rank), ubMaps(rank);
-  for (unsigned i = 0; i < rank; ++i)
+  for (unsigned i = 0; i < rank; ++i) {


Duplicate with #127809.

This change has been merged. The first commit can be dropped during rebase.

bondhugula · 2025-02-20T03:58:10Z

@bondhugula I did was able to successfully rebase on my local without conflicts:
Any suggestions?

#127809 has been merged into main now. Please rebase and test.

bondhugula · 2025-02-20T23:55:38Z

@bondhugula I did was able to successfully rebase on my local without conflicts:
Any suggestions?

#127809 has been merged into main now. Please rebase and test.

You can simply do git pull <upstream> main --rebase and then do a force push (to your fork branch). Then squash and merge from here.

ayokunle321 · 2025-02-21T00:57:56Z

You can simply do git pull <upstream> main --rebase and then do a force push (to your fork branch). Then squash and merge from here.

So I just did that exactly on my local, but it's still saying saying merging is blocked - and I don't have any merge commits. Have no clue why.

bondhugula · 2025-02-22T06:03:06Z

You can simply do git pull <upstream> main --rebase and then do a force push (to your fork branch). Then squash and merge from here.

So I just did that exactly on my local, but it's still saying saying merging is blocked - and I don't have any merge commits. Have no clue why.

You need to have selected "Rebase branch" among the two options on the side. I did it and it had not issue squash and merge.

llvmbot added mlir:affine mlir labels Feb 13, 2025

ayokunle321 changed the title ~~[mlir][linalg} Add check for affine.for map bounds~~ [mlir][linalg] Add check for 'affine.for' map bound results Feb 13, 2025

krzysz00 approved these changes Feb 13, 2025

View reviewed changes

mlir/lib/Dialect/Affine/IR/AffineOps.cpp Outdated Show resolved Hide resolved

mlir/lib/Dialect/Affine/IR/AffineOps.cpp Outdated Show resolved Hide resolved

ayokunle321 changed the title ~~[mlir][linalg] Add check for 'affine.for' map bound results~~ [mlir][affine] Add Check for 'affine.for' Bound Map Results Feb 13, 2025

ayokunle321 mentioned this pull request Feb 15, 2025

[Mlir][affine] --lower-affine crashes in AffineOps.cpp:foldLoopBounds #120001

Closed

bondhugula approved these changes Feb 18, 2025

View reviewed changes

mlir/lib/Dialect/Affine/IR/AffineOps.cpp Show resolved Hide resolved

ayokunle321 force-pushed the affine-loop-bounds-check branch from 001b1ff to b07e112 Compare February 19, 2025 19:12

CoTinker reviewed Feb 20, 2025

View reviewed changes

ayokunle321 force-pushed the affine-loop-bounds-check branch 2 times, most recently from 0790e02 to 64b6566 Compare February 21, 2025 00:50

ayokunle321 added 6 commits February 20, 2025 17:52

added check for affine.for bounds

b59c62e

added comments

7aa091f

fixed code style

b95f0e5

fix style

302c2ec

format code properly

20a24fc

removed unnecessary comments

6cead0b

ayokunle321 force-pushed the affine-loop-bounds-check branch from 64b6566 to 6cead0b Compare February 21, 2025 00:53

bondhugula merged commit dc72a93 into llvm:main Feb 22, 2025
11 checks passed

[mlir][affine] Add Check for 'affine.for' Bound Map Results #127105

[mlir][affine] Add Check for 'affine.for' Bound Map Results #127105

Uh oh!

Conversation

ayokunle321 commented Feb 13, 2025 • edited by bondhugula Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Feb 13, 2025

Uh oh!

llvmbot commented Feb 13, 2025

Uh oh!

github-actions bot commented Feb 13, 2025

Uh oh!

github-actions bot commented Feb 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

krzysz00 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bondhugula commented Feb 18, 2025

Uh oh!

bondhugula commented Feb 18, 2025

Uh oh!

ayokunle321 commented Feb 18, 2025

Uh oh!

bondhugula commented Feb 19, 2025

Uh oh!

bondhugula commented Feb 19, 2025

Uh oh!

bondhugula commented Feb 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ayokunle321 commented Feb 19, 2025

Uh oh!

CoTinker Feb 20, 2025

Choose a reason for hiding this comment

Uh oh!

bondhugula Feb 20, 2025

Choose a reason for hiding this comment

Uh oh!

bondhugula commented Feb 20, 2025

Uh oh!

bondhugula commented Feb 20, 2025

Uh oh!

ayokunle321 commented Feb 21, 2025

Uh oh!

Uh oh!

bondhugula commented Feb 22, 2025

Uh oh!

Uh oh!

ayokunle321 commented Feb 13, 2025 •

edited by bondhugula

Loading

github-actions bot commented Feb 13, 2025 •

edited

Loading

bondhugula commented Feb 19, 2025 •

edited

Loading