This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Change the syntax of AffineMapAttr and IntegerSetAttr to avoid conflicts with function types.
ClosedPublic

Authored by rriddle on Jan 8 2020, 5:33 PM.

Details

Summary

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<>.

Diff Detail

Event Timeline

rriddle created this revision.Jan 8 2020, 5:33 PM
Herald added 1 blocking reviewer(s): nicolasvasilache. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
nicolasvasilache accepted this revision.Jan 8 2020, 6:00 PM

This is most welcome, thanks!

This revision is now accepted and ready to land.Jan 8 2020, 6:00 PM

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) : ...

rriddle added a comment.EditedJan 8 2020, 8:29 PM

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

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.

ftynse added a subscriber: ftynse.Jan 9 2020, 1:40 AM

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".

rriddle updated this revision to Diff 237119.Jan 9 2020, 10:05 AM

Address comments.

rriddle marked an inline comment as done.Jan 9 2020, 10:06 AM

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.

+1 This is exactly what I was thinking.

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

rriddle updated this revision to Diff 237148.Jan 9 2020, 12:00 PM

Rebase on recent changes.

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

ftynse accepted this revision.Jan 9 2020, 2:04 PM

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.

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

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

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

This revision was automatically updated to reflect the committed changes.

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.

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?