The current syntax for AffineMapAttr and IntegerSetAttr conflict with function types, making it currently impossible to round-trip function types(and e.g. FuncOp) in the IR. This revision changes the syntax for the attributes by wrapping them in a keyword. AffineMapAttr is wrapped with affine_map<> and IntegerSetAttr is wrapped with affine_set<>.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
Unit tests: fail. 61168 tests passed, 1 failed and 728 were skipped.
failed: libc++.std/thread/thread_mutex/thread_mutex_requirements/thread_mutex_requirements_mutex/thread_mutex_class/try_lock.pass.cpp
clang-tidy: fail. Please fix clang-tidy findings.
clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.
Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml
How about just using a space after the keyword instead of angular brackets? The angular bracket right before/after the parenthesis looks a bit clumsy, and there is no specialization being implied on the map/set. It's a character less with the space FWIW. And we could drop affine_ as well - those are the only maps and sets, and it prevents repetition at several places (like affine.apply affine_map, affine.if affine_set ..., affine.for %i = affine_map...).
With the space and affine_ dropped:
affine.for %j = 0 to map (d0) -> (d0 - d0 mod 4) (%i)
#map1 = map (d0, d1) -> ((d0 * 2048 + d1 * 256) floordiv 32)
memref<512 x 512 x i32, map (d0, d1) -> (d0, d1), 2>
affine.if set (d0, d1) : ...
I don't see it as a specialization, it is a grouping mechanism that clearly delineate the scope of the attribute. We use this in many other places and doesn't imply that. I personally find the space to create a clumsier syntax as it makes it more difficult for me to tell what is actually part of the syntax. For example, there are many cases where we end up something like () : () () or (...) -> (...) (...). For me it is much easier to read something like: set<() : ()> () or map<() -> ()> (). Is there some other delimiter grouping that would be less clumsy for you? {} perhaps? set{() : ()}()/map{() -> ()}()?
map/set. It's a character less with the space FWIW. And we could drop affine_ as well - those are the only maps and sets, and it prevents repetition at several places (like affine.apply affine_map, affine.if > affine_set ..., affine.for %i = affine_map...).
Happy to drop the 'affine_', but it should be noted that this only removed ambiguity for the affine dialect. There are other users of these attributes that don't have this problem.
I support the reasoning for having delimiters around the actual map. I've always found the cases of immediate inline application confusing, especially for maps: (d0)[s0] -> (d0,s0)(%0)[%1].
I am also supportive of keeping the affine_ prefix in the name, since it clearly indicates the affine-ness of the attribute, e.g. the kind of sets/maps that can be expressed. We may eventually consider other similar attributes, like permutation-only maps, that could be clearly denoted now. I don't think repetition is a big problem, we are doing an IR after all, and we repeat types for all uses of a value, for example. If it proves to be annoying, we could consider having a special syntax for affine loops and branches that supports "inline" maps and sets without the affine_map token at all. We already have special syntax for affine loads and stores anyway, so it can be adapted and reused.
mlir/docs/Dialects/Affine.md | ||
---|---|---|
302 | This looks like it would take an unprefixed affine map. There may be other places that say they consume "affine-map" rather than "affine-map-attribute". |
Unit tests: pass. 61499 tests passed, 0 failed and 769 were skipped.
clang-tidy: pass.
clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.
Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml
Unit tests: pass. 61656 tests passed, 0 failed and 778 were skipped.
clang-tidy: fail. Please fix clang-tidy findings.
clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.
Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml
I don't think repetition is a big problem, we are doing an IR after all, and we repeat types for all uses of a value, for example. If it proves to be
The type is printed for each use because one'd like to know about the value used by looking at the op locally and without having to search for its definition -- the latter would be a readability killer.
Hmm... I didn't see the delineation to be an issue because wherever the map typically appears, it either ends a line (in outlined definitions), has a trailing comma or '>', or often has a non-empty operand list following (%x, %y).
Is there some other delimiter grouping that would be less clumsy for you? {} perhaps? set{() : ()}()/map{() -> ()}()?
I think the angular brackets are better or as good as any other delimiter.
map/set. It's a character less with the space FWIW. And we could drop affine_ as well - those are the only maps and sets, and it prevents repetition at several places (like affine.apply affine_map, affine.if > affine_set ..., affine.for %i = affine_map...).
Happy to drop the 'affine_', but it should be noted that this only removed ambiguity for the affine dialect. There are other users of these attributes that don't have this problem.
Note that the ones I listed above are the most common users; those that appear as attributes on ops without a custom form are far fewer, and those aren't an issue as well when outlined due to the #map and #set prefix.
This is all really a bikeshed.
Unit tests: pass. 61744 tests passed, 0 failed and 780 were skipped.
clang-tidy: unknown.
clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.
Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml
I thought the type is printed for each use because they could appear before the def in a CFG?
Most programming languages are including the type for variable definitions only and not every use, yet they are readable, what is the fundamental difference you see in the IR?
This looks like it would take an unprefixed affine map. There may be other places that say they consume "affine-map" rather than "affine-map-attribute".