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

Add aievec dialect, associated optimizations, and aievec-to-cpp transl… #81

Merged
merged 11 commits into from
Jan 27, 2022

Conversation

pssrawat
Copy link
Contributor

…ation. This allows generation of autovectorized AIE kernels from scalar kernels written in affine dialect

…ation. This allows generation of autovectorized AIE kernels from scalar kernels written in affine dialect
Copy link
Collaborator

@stephenneuendorffer stephenneuendorffer left a comment

Choose a reason for hiding this comment

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

It doesn't seem like you have any true unit-tests... At best you're testing combinations of affine-super-vectorize and --aie-vectorize. Can you add some tests that are pure unit tests?

Please try to address as much of the feedback as you have time to.

Comment on lines 3 to 8
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
// (c) Copyright 2022 Xilinx Inc.
//
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not part of the LLVM project. Please use the copyright in the mlir-aie repo

Comment on lines 69 to 72
inline VectorType getVectorType(Value val) {
assert(val.getType().isa<VectorType>());
return val.getType().cast<VectorType>();
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

unnecessary.

Comment on lines 119 to 122
Type getLHSType() { return lhs().getType(); }
Type getRHSType() { return rhs().getType(); }
Type getResultType() { return result().getType(); }
}];
Copy link
Collaborator

Choose a reason for hiding this comment

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

unnecessary.

Results<(outs AIEVec_Acc:$result)> {
let summary = "AIE ups";
let description = [{
Xilinx-specific ups intrinsic. Moves data from AIE vector data type
Copy link
Collaborator

Choose a reason for hiding this comment

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

ups -> upshift

}];
let extraClassDeclaration = [{
size_t getSourceSize() { return sources().size(); }
Type getSourceType() { return sources().getTypes().front(); }
Copy link
Collaborator

Choose a reason for hiding this comment

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

What requires the types to be the same?

Comment on lines +63 to +64
if (!resultType)
return upd.emitError("requires vector type");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should come from tablegen?

Comment on lines +315 to +320
if (result.attributes.getAttrs().size() != 1)
return parser.emitError(typesLoc, "requires one attribute");

// Assert that there are two types (source vector and accumulator result)
if (types.size() != 2)
return parser.emitError(typesLoc, "requires two types");
Copy link
Collaborator

Choose a reason for hiding this comment

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

This kind of thing should come from the tablegen spec.

Comment on lines 323 to 328
VectorType vectorType = types[0].dyn_cast<VectorType>();
if (!vectorType)
return parser.emitError(typesLoc, "requires vector type");
aievec::AccType accType = types[1].dyn_cast<aievec::AccType>();
if (!accType)
return parser.emitError(typesLoc, "requires accumulator type");
Copy link
Collaborator

Choose a reason for hiding this comment

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

should come from tablegen

Comment on lines 543 to 559
if (IntegerType itype = ltype.dyn_cast<IntegerType>())
ltypeWidth = itype.getWidth();
else if (FloatType ftype = ltype.dyn_cast<FloatType>())
ltypeWidth = ftype.getWidth();

if (IntegerType itype = rtype.dyn_cast<IntegerType>())
rtypeWidth = itype.getWidth();
else if (FloatType ftype = rtype.dyn_cast<FloatType>())
rtypeWidth = ftype.getWidth();

if (IntegerType itype = atype.dyn_cast<IntegerType>())
atypeWidth = itype.getWidth();
else if (FloatType ftype = atype.dyn_cast<FloatType>())
atypeWidth = ftype.getWidth();

if (ltypeWidth == 0 || rtypeWidth == 0 || atypeWidth == 0)
return mul.emitError("failed to find width of the vector or accumulator");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Lots of duplication here.

Comment on lines +276 to +280
bool isMulOrFMAOp = isa<MulIOp, MulFOp, vector::FMAOp>(Op);
bool isSubOrAddOp = isa<SubIOp, SubFOp, AddIOp, AddFOp>(Op);
if (!isMulOrFMAOp && !isSubOrAddOp)
return true;

Copy link
Collaborator

Choose a reason for hiding this comment

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

This could be an MLIR trait?

@pssrawat pssrawat changed the title Add aievec dialec, associated optimizations, and aievec-to-cpp transl… Add aievec dialect, associated optimizations, and aievec-to-cpp transl… Jan 26, 2022
@stephenneuendorffer stephenneuendorffer enabled auto-merge (squash) January 27, 2022 21:08
auto-merge was automatically disabled January 27, 2022 21:14

Head branch was pushed to by a user without write access

@stephenneuendorffer stephenneuendorffer enabled auto-merge (squash) January 27, 2022 21:16
@stephenneuendorffer stephenneuendorffer merged commit 440ea27 into Xilinx:main Jan 27, 2022
fifield added a commit to fifield/mlir-aie that referenced this pull request Nov 8, 2023
* Add addWeighted placeholder kernel test

* Enable add_weighted_v0 rtp
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.

2 participants