[MLIR][Quant] Add alias type printing for BlockFloat#803
[MLIR][Quant] Add alias type printing for BlockFloat#803jorickert wants to merge 2 commits intoaie-publicfrom
Conversation
jorickert
requested review from
ehsan-toosi,
ljfitz,
roberteg16 and
ttjost
as code owners
|
|
||
| // ----- | ||
|
|
||
| // CHECK: !bfp16_bf = !quant.block_float<mode=BFP16, axis=1> |
There was a problem hiding this comment.
I am not very happy with the _bf suffix. As mlir does not allow an alias to end in a number, another approach would be to use _ as suffix or adjust MLIR to allow aliases to end in a number
There was a problem hiding this comment.
Do you know if there is a hard requirement why this constraint is enforced? For types, it's quite common to end in numbers.
Otherwise, for me it's fine the way it is.
A few other suggestions just for fun:
mx6t(t for "type")mx6+mx6!
There was a problem hiding this comment.
In commit b9f07f40ae3b074f42203491d22a83aa2670864c (i plan to upstream this one) i lifted the restriction that alias can not end in a number. Now it can simply be mx6 and bfp16
gerion-amd
left a comment
gerion-amd
left a comment
There was a problem hiding this comment.
Thanks. I think, it's much easier to read with that.
| if (!blockType) | ||
| return AliasResult::NoAlias; | ||
| if (blockType.getAxis() != 1) | ||
| return AliasResult::NoAlias; |
There was a problem hiding this comment.
Maybe we want this to encode the axis in the type (e.g. mx6_1bf) some time in the future, but I see no blocker for this with the current design.
|
|
||
| // ----- | ||
|
|
||
| // CHECK: !bfp16_bf = !quant.block_float<mode=BFP16, axis=1> |
There was a problem hiding this comment.
Do you know if there is a hard requirement why this constraint is enforced? For types, it's quite common to end in numbers.
Otherwise, for me it's fine the way it is.
A few other suggestions just for fun:
mx6t(t for "type")mx6+mx6!
ttjost
left a comment
ttjost
left a comment
There was a problem hiding this comment.
Could you check how ! would look like as the last character? It looks prettier than what you have right now, IMO.
jorickert
force-pushed
the
jrickert.mx6_alias
branch
from
486ada0 to
6eb12a5
Compare
…with a digit. To prevent collisions when multiple aliases are registered with the same name, keep track of all used aliases and skip used ones when generating conflict-resolving suffixes. This does not affect the reading of .mlir files, but will affect the printing of aliases that end in a digit, as they won't have an _ appended anymore. Signed-off-by: Jonas Rickert <jonas.rickert@amd.com>
Signed-off-by: Jonas Rickert <jonas.rickert@amd.com>
jorickert
force-pushed
the
jrickert.mx6_alias
branch
from
6eb12a5 to
f3caaa5
Compare
ttjost
left a comment
ttjost
left a comment
There was a problem hiding this comment.
I wonder if disambiguation is the only reason for not having the support for trailing digits in the first place. If it is, then LGTM, but I think it makes more sense to open a PR upstream to get an idea of why we didn't allow it in the first place. Then we can merge this safely.
Upstream completely changed the implementation of the alias printing, its now part of the type not dialect. :-/ |
What is the consequence? Should we bump first or still merge? If bumping is not an option in the near future, I would still merge it, if we can achieve the same MLIR syntax after the bump. |
3 participants
No description provided.