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

Adding type constraints for aievec::mul_elem operation #1514

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

muradq-amd
Copy link
Collaborator

  • The previous PR was automatically closed when I tried to hard rest my branch to revert unwanted changes in the [platform/boards] submodule. Here is the link to the previous PR for reference:
  • I rewrote the type constraints for the aievec::mul_elem operation in a simpler way, as we discussed in the last meeting.
  • Changes:
    • Use operand type constraints for lhs, rhs, and acc.
    • Use already implemented predicates in:
      • llvm-project/mlir/include/mlir/IR/OpBase.td
      • llvm-project/mlir/include/mlir/IR/CommonTypeConstraints.td
  • Added Constraints:
    • lhs and rhs operands must match shape.
    • lhs and rhs operands must match type.
    • IsValidAccTypeForAIE2MulElem.

Please let me know if there is a better way to revise these changes.

Comment on lines 334 to 344
`vector<32xi8>` | `vector<32xi8>` | `vector<32xi8>`
`vector<32xi8>` | `vector<32xi8>` | `vector<32xi32>`
`vector<64xi8>` | `vector<64xi8>` | `vector<32xi32>`
`vector<32xi16>` | `vector<32xi16>` | `vector<32xi16>`
`vector<32xi16>` | `vector<32xi16>` | `vector<32xi32>`
`vector<32xi16>` | `vector<32xi16>` | `vector<16xi64>`
`vector<16xi32>` | `vector<16xi32>` | `vector<16xi32>`
`vector<16xi32>` | `vector<16xi32>` | `vector<16xi64>`
`vector<16xbf16>` | `vector<16xbf16>` | `vector<16xbf16>`
`vector<16xbf16>` | `vector<16xbf16>` | `vector<16xf32>`
`vector<16xf32>` | `vector<16xf32>` | `vector<16xf32>`'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should just have:

        `vector<32xi8>`    | `vector<32xi8>`    | `vector<32xi32>`
        `vector<32xi16>`   | `vector<32xi16>`   | `vector<32xi32>`
        `vector<16xi32>`   | `vector<16xi32>`   | `vector<16xi64>`
        `vector<16xbf16>`  | `vector<16xbf16>`  | `vector<16xf32>`
        `vector<16xf32>`   | `vector<16xf32>`   | `vector<16xf32>`

These operations refer to the following intrinsics in the table at https://www.xilinx.com/htmldocs/xilinx2023_2/aiengine_ml_intrinsics/intrinsics/group__intr__gpvectorop__mul.html.

8-bit x 8-bit = 32-bit,	32,	1x2,	2x1,	1x1
16-bit x 16-bit = 32-bit,	32,	1x1,	1x1,	1x1
bfloat16 x bfloat16 = fp32,	16,	1x2,	2x1,	1x1

The two more are at https://www.xilinx.com/htmldocs/xilinx2023_2/aiengine_ml_intrinsics/intrinsics/group__intr__gpvectorop__emul__32bx32b.html#gae6f04990833f0cf4b62bf1433ac11e6a and
https://www.xilinx.com/htmldocs/xilinx2023_2/aiengine_ml_intrinsics/intrinsics/group__intr__gpvectorop__mul__fp32xfp32.html#gac1e9a132ee05edf8b6afe9c985598768

We can support arith.muli/arith.mulf in different result types (as you listed above), because we add aievec.cast/aievec.srs to cast the acc types back to normal integer/floating types. However, that shouldn't be part of the semantic of aievec.mul_elem.

Comment on lines 149 to 150
class hasAnyTypeOf<string name, list<Type> vaildTypes> :
SubstLeaves<"$_self", ElementType<name>.result, AnyTypeOf<vaildTypes>.predicate>;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An equivalent predicate exists upstream already.

Comment on lines 156 to 160
And <[hasAnyTypeOf<lhs, [I8 ]>, hasAnyTypeOf<acc, [I8, I32]>]>,
And <[hasAnyTypeOf<lhs, [I16]>, hasAnyTypeOf<acc, [I16, I32, I64]>]>,
And <[hasAnyTypeOf<lhs, [I32]>, hasAnyTypeOf<acc, [I32, I64]>]>,
And <[hasAnyTypeOf<lhs, [BF16]>, hasAnyTypeOf<acc, [BF16, F32]>]>,
And <[hasAnyTypeOf<lhs, [F32]>, hasAnyTypeOf<acc, [F32]>]>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Notice James' commit above.

SubstLeaves<"$_self", ElementType<name>.result, AnyTypeOf<vaildTypes>.predicate>;

class IsValidAccTypeForAIE2MulElem<string lhs, string rhs, string acc> :
PredOpTrait<lhs # " x " # rhs # " = " # acc # " is a valid AIE2 " #
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not very descriptive of what's being checked.

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.

None yet

3 participants