Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rename unroll_n_to_subgroups to subgroups_n #19102

Merged
merged 1 commit into from
Nov 13, 2024

Conversation

bjacob
Copy link
Contributor

@bjacob bjacob commented Nov 11, 2024

"Unroll" usually means "generate more instructions", so the terminology being changed here, unroll_n_to_subgroups, created confusion.

@qedawkins
Copy link
Contributor

distribute_to_subgroups might be more canonical w.r.t. naming in the rest of the compiler.

@bjacob
Copy link
Contributor Author

bjacob commented Nov 11, 2024

Interesting. I parsed "distribute to subgroups" as the action of distributing to the preestablished number of subgroups, while here, the effect is to set/increase the number of subgroups. WDYT?

@qedawkins
Copy link
Contributor

Hmmm I parsed the field as "this dimension will be distributed to subgroups" as an analog to "this dimension will be unrolled to more instructions"

@bjacob
Copy link
Contributor Author

bjacob commented Nov 11, 2024

Yeah but the nuance is that this field is what causes this dimension to be created in the first place.

@qedawkins
Copy link
Contributor

I see, so really we have two distinct names occupying the same field of the attribute. Before MaterializeEncodings it is a yet-to-be introduced dimension and is expanding to subgroups like you're saying. After MaterializeEncodings this becomes a concrete dimensions that is going to be distributed.

I would think that the name of the field in the DataTiledMMAAttribute, since that attribute is only materialized in IR after MaterializeEncodings, should be distribute_to_subgroups, whereas if we want we can make the names of C++ referencing subgroups expand_to_subgroups. That's my intuition at least, but I won't be picky here if you insist on this name (because it's better than unroll).

@bjacob
Copy link
Contributor Author

bjacob commented Nov 11, 2024

Before MaterializeEncoding, the attribute does not exist yet. MaterializeEncoding is where it is originally created.

return DataTiledMMAAttr::get(ctx, intrinsicMma.getIntrinsic(), unrollM,
. That is also when the extra tensor dimensions are created: these two aspects are simultaneous, as MaterializeEncoding is a type-conversion pass, and that is necessary anyway to have well-formed multi_mma ops.

From MaterializeEncoding up until DistributeMmaToLanes, the DataTiledMMAAttr attribute carries those unroll_{m,n}_to_subgroups parameters. These are dropped at DistributeMmaToLanes, as the IR after that point is single-thread IR where these subgroup parameters are no longer relevant.

@bjacob
Copy link
Contributor Author

bjacob commented Nov 11, 2024

Just discussed online with @qedawkins and we agreed on subgroups_m, subgroups_n.

@bjacob bjacob force-pushed the users/bjacob/cdna1-cdna2-data-tiling branch from 58beb25 to a89bc41 Compare November 11, 2024 21:57
@bjacob bjacob force-pushed the users/bjacob/rename-expand-to-subgroups branch 2 times, most recently from f58d8cb to 82cbb2f Compare November 11, 2024 22:06
@bjacob bjacob changed the base branch from users/bjacob/cdna1-cdna2-data-tiling to users/bjacob/simplify-unroll November 11, 2024 22:07
@bjacob bjacob marked this pull request as ready for review November 12, 2024 16:13
@bjacob bjacob force-pushed the users/bjacob/simplify-unroll branch from 7c3f370 to d0eb097 Compare November 12, 2024 16:25
@bjacob bjacob requested a review from benvanik as a code owner November 12, 2024 16:25
@bjacob bjacob force-pushed the users/bjacob/rename-expand-to-subgroups branch from 82cbb2f to b69eb02 Compare November 12, 2024 16:26
@bjacob bjacob changed the title Rename "unroll to subgroups" to "expand to subgroups" Rename unroll_n_to_subgroups to subgroups_n Nov 12, 2024
@hanhanW
Copy link
Contributor

hanhanW commented Nov 12, 2024

Just discussed online with @qedawkins and we agreed on subgroups_m, subgroups_n.

Do you also need to update the PR description?

@bjacob bjacob force-pushed the users/bjacob/simplify-unroll branch from d0eb097 to 47232f2 Compare November 12, 2024 19:35
@bjacob bjacob force-pushed the users/bjacob/rename-expand-to-subgroups branch from b69eb02 to 7ffd79a Compare November 12, 2024 19:38
@bjacob bjacob force-pushed the users/bjacob/simplify-unroll branch from 47232f2 to 3081305 Compare November 13, 2024 18:32
@bjacob bjacob force-pushed the users/bjacob/rename-expand-to-subgroups branch from 7ffd79a to 1f71536 Compare November 13, 2024 18:33
Base automatically changed from users/bjacob/simplify-unroll to main November 13, 2024 19:07
bjacob added a commit that referenced this pull request Nov 13, 2024
In `getIntrinsicSwizzle`, we had a slightly roundabout way of
constructing the swizzle from the `SingleSubgroupLayout`. We started
from the `thread` dims, which we used unconditionally even if they had
the value 1, leading to unit dims; and then we inserted the `element`
dims *on the inside*, which required custom manipulation of the
`swizzle` field. Now we just start from the `element` dims and work our
way outwards from there, which means we can reuse the same helper that
used to be named `unroll` and that we rename here to `expand` in
preparation for #19102, and which
we also move to be a `static` helper since it's no longer used outside
of this file.

---------

Signed-off-by: Benoit Jacob <[email protected]>
Signed-off-by: Benoit Jacob <[email protected]>
@bjacob bjacob force-pushed the users/bjacob/rename-expand-to-subgroups branch from 1f71536 to b318881 Compare November 13, 2024 21:05
@bjacob bjacob enabled auto-merge (squash) November 13, 2024 21:05
@bjacob bjacob merged commit cb5d1ab into main Nov 13, 2024
39 checks passed
@bjacob bjacob deleted the users/bjacob/rename-expand-to-subgroups branch November 13, 2024 22:08
Groverkss pushed a commit to Groverkss/iree that referenced this pull request Dec 1, 2024
…19105)

In `getIntrinsicSwizzle`, we had a slightly roundabout way of
constructing the swizzle from the `SingleSubgroupLayout`. We started
from the `thread` dims, which we used unconditionally even if they had
the value 1, leading to unit dims; and then we inserted the `element`
dims *on the inside*, which required custom manipulation of the
`swizzle` field. Now we just start from the `element` dims and work our
way outwards from there, which means we can reuse the same helper that
used to be named `unroll` and that we rename here to `expand` in
preparation for iree-org#19102, and which
we also move to be a `static` helper since it's no longer used outside
of this file.

---------

Signed-off-by: Benoit Jacob <[email protected]>
Groverkss pushed a commit to Groverkss/iree that referenced this pull request Dec 1, 2024
"Unroll" usually means "generate more instructions", so the terminology
being changed here, `unroll_n_to_subgroups`, created confusion.

Signed-off-by: Benoit Jacob <[email protected]>
giacs-epic pushed a commit to giacs-epic/iree that referenced this pull request Dec 4, 2024
…19105)

In `getIntrinsicSwizzle`, we had a slightly roundabout way of
constructing the swizzle from the `SingleSubgroupLayout`. We started
from the `thread` dims, which we used unconditionally even if they had
the value 1, leading to unit dims; and then we inserted the `element`
dims *on the inside*, which required custom manipulation of the
`swizzle` field. Now we just start from the `element` dims and work our
way outwards from there, which means we can reuse the same helper that
used to be named `unroll` and that we rename here to `expand` in
preparation for iree-org#19102, and which
we also move to be a `static` helper since it's no longer used outside
of this file.

---------

Signed-off-by: Benoit Jacob <[email protected]>
Signed-off-by: Giacomo Serafini <[email protected]>
giacs-epic pushed a commit to giacs-epic/iree that referenced this pull request Dec 4, 2024
"Unroll" usually means "generate more instructions", so the terminology
being changed here, `unroll_n_to_subgroups`, created confusion.

Signed-off-by: Benoit Jacob <[email protected]>
Signed-off-by: Giacomo Serafini <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants