-
Notifications
You must be signed in to change notification settings - Fork 17
Use IREE's MLIR builder Python bindings for gemmbench IR generation #57
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
Conversation
|
(This is stacked on top of #56, please merge that one first.) |
kuhar
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall. Switching to a nightly iree realease should fix the issue with matmul constructor.
0e0d767 to
7887185
Compare
7887185 to
4256a8c
Compare
4256a8c to
33dd27c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good % one minor issue
33dd27c to
ca3b56f
Compare
tests/test_gemmbench_mlir_gen.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there some constructor that doesn't produce these cast attributes? I think it's fine as-is and being explicit doesn't hurt, but it does look odd since nothing else in IREE produces these
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll investigate it, I'm curious myself as to what this means.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it can be done with the constructor currently in use here, it takes a TypeFn for the cast argument and that can be either cast_signed or cast_unsigned, there's no other options (I tried None and creating an empty TypeFn, neither were accepted). I tried doing this instead:
acc = linalg.MatmulTransposeAOp(inputs = [arg0, arg1], outputs = [filled_tensor], result_tensors = [acc_type])But it seems like that produces an invalid operation; the IR prints weirdly after I do that:
%3 = "linalg.matmul_transpose_a"(%arg0, %arg1, %2) <{operandSegmentSizes = array<i32: 2, 1>}> ({
}) {linalg.memoized_indexing_maps = [#map, #map1, #map2]} : (tensor<5120x32000xbf16>, tensor<5120x1xbf16>, tensor<32000x1xf32>) -> tensor<32000x1xf32>
And I get this error if I call .verify():
E iree.compiler._mlir_libs._site_initialize.<locals>.MLIRError: Verification failed:
E error: "gemm_32000_1_5120_f16_f32_tA": 'linalg.matmul_transpose_a' op expects to have 1 region with 1 block
E note: "gemm_32000_1_5120_f16_f32_tA": see current operation:
E %3 = "linalg.matmul_transpose_a"(%arg0, %arg1, %2) <{operandSegmentSizes = array<i32: 2, 1>}> ({
E }) {linalg.memoized_indexing_maps = [affine_map<(d0, d1, d2) -> (d2, d0)>, affine_map<(d0, d1, d2) -> (d2, d1)>, affine_map<(d0, d1, d2) -> (d0, d1)>]} : (tensor<5120x32000xf16>, tensor<5120x1xf16>, tensor<32000x1xf32>) -> tensor<32000x1xf32>
There's probably some way to repair it but I'm not sure if it's worth the effort. Maybe I can ask for a second opinion from someone who knows linalg better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, ask on IREE discord.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For future reference, discussion here: https://discord.com/channels/689900678990135345/689900680009482386/1352681987726639154
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we file an issue against mlir (upstream)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't feel strongly either way on that, maybe @rkayaith or @makslevental have an opinion there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the python bindings are doing anything wrong necessarily, the C++ side defaults to cast_signed as well:
// LinalgNamedStructuredOps.yamlgen.td
def MatmulTransposeBOp : LinalgStructuredBase_Op<"matmul_transpose_b", !listconcat([AttrSizedOperandSegments],
/*extraInterfaces=*/[LinalgContractionOpInterface])> {
...
let arguments = (ins
Variadic<AnyType>:$inputs,
Variadic<AnyShaped>:$outputs,
DefaultValuedOptionalAttr<TypeFnAttr, "TypeFn::cast_signed">:$cast
);
...
But unfortunately using DefaultValuedOptionalAttr causes the attribute to only be elided in the asm format when the attribute is missing. IMO using DefaultValuedAttr instead (no Optional) + updating the custom asm format to elide the default would be a good solution here, but I don't know if there'd be any repercussions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created llvm/llvm-project#132961 now. I am not very confident I have described the issue well, so if anyone thinks they can add something by commenting there, I would appreciate it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you cc the people who made this change? Otherwise they may not ever see this issue.
Resolves nod-ai#52. The README is changed to suggest using the pre-release IREE builds (the CI already used them), because the linalg.matmul constructor is broken in the latest stable IREE build, but not on the latest IREE.
ca3b56f to
baddea3
Compare
Resolves #52.
The README is changed to suggest using the pre-release IREE builds (the CI already used them), because the linalg.matmul constructor is broken in the latest stable IREE build, but not on the latest IREE.