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

[aievec] Move trunc/ext to common lowering #1437

Merged
merged 1 commit into from
May 8, 2024

Conversation

jsetoain
Copy link
Collaborator

Currently, trunc/ext ops get lowered in a separate pass with two simple patterns. This is mostly because other patterns check their users for these ops, which makes all the patterns involved dependent on each other and their order of application.

This patch addresses this issue and moves these two last patterns together with all the other vector-to-aievec patterns.

Copy link
Collaborator

@david-vc david-vc left a comment

Choose a reason for hiding this comment

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

Looks good to me, but I did have a few questions in the comments.


if (auto srsOp = dyn_cast<aievec::SRSOp>(op)) {
auto srsOpSrcOp = srsOp.getSource().getDefiningOp();
if (isa<aievec::UPSOp>(srsOpSrcOp) || isa<aievec::CastOp>(srsOpSrcOp))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suppose that we have UPS from int8 => int32 followed by SRS from int32 => int16.
Would it be OK to return true in this case?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think we should because that's not a narrowing operation (int8 -> int16), but in this particular case it's looking for an SRS followed by either a UPS or a aievec.cast, which are the two possible conversion for trunc ops, depending on the operand types.

// 2) Move from accumulator: aievec.srs
auto srsSource = srsOp.getSource();
if (srsSource)
if (auto upsOp = srsSource.getDefiningOp<aievec::UPSOp>())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suppose that we have UPS from int16 => int32 followed by SRS from int32 to int8.
IMO, we should return sct::optional{} in this case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Indeed, that would be the thing to do since that's a narrowing operation, not a widening one. But we're not looking for UPS -> SRS sequences here, only SRS -> UPS.

@@ -2757,6 +2799,98 @@ static void configureAIEVecCommonLegalizations(ConversionTarget &target,
target.addIllegalOp<vector::TransferReadOp>();
}
target.addIllegalOp<vector::ExtractStridedSliceOp>();

// ****************************NEW****************************
Copy link
Collaborator

Choose a reason for hiding this comment

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

Line 2803 is probably unintended for this PR.

@@ -2757,6 +2799,98 @@ static void configureAIEVecCommonLegalizations(ConversionTarget &target,
target.addIllegalOp<vector::TransferReadOp>();
}
target.addIllegalOp<vector::ExtractStridedSliceOp>();

// ****************************NEW****************************
target.addDynamicallyLegalOp<arith::ExtFOp>([](arith::ExtFOp extfOp) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

So basically this is saying that extf ops are legal, unless that they are from v16 bfloat16 to v16 f32.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I honestly don't know what the logic is here, I haven't changed the semantics of what we do with these ops, just where it happens and what assumptions they're making about order and other patterns.

It'd probably be a good idea, at some point, to take a good look at all these casts (and movements between accumulator/regular register files) and see what needs to be done. There's probably a sensible way to not do any of this, and simplify the conversion.

Copy link
Collaborator

@jamestcl-amd jamestcl-amd left a comment

Choose a reason for hiding this comment

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

The test for this conversion pattern is missing from https://github.com/Xilinx/mlir-aie/tree/main/test/Conversion/VectorToAIEVec... It might be beneficial to create it this time.

LowerVectorTransferReadToAIEUPD
>(patterns.getContext(), 128, 1024, 256, 1024);
patterns.add<
LowerVectorAddIOpToAIEVecAddElemOp
Copy link
Collaborator

@jamestcl-amd jamestcl-amd Apr 30, 2024

Choose a reason for hiding this comment

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

We can move below patterns to here as well.

LowerVectorSubIOpToAIEVecSubElemOp,
LowerVectorAddFOpToAIEVecAddElemOp,
LowerVectorSubFOpToAIEVecSubElemOp,

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Aren't those applicable for LLVMIR conversion as well?

@jsetoain jsetoain force-pushed the move-ext-to-common-lowering branch 2 times, most recently from 5e40b95 to efb0a87 Compare May 7, 2024 14:24
Currently, trunc/ext ops get lowered in a separate pass with two simple
patterns. This is mostly because other patterns check their users for
these ops, which makes all the patterns involved dependent on each other
and their order of application.

This patch addresses this issue and moves these two last patterns
together with all the other vector-to-aievec patterns.
@jsetoain jsetoain force-pushed the move-ext-to-common-lowering branch from efb0a87 to 1d4dd09 Compare May 8, 2024 08:28
@jsetoain jsetoain enabled auto-merge May 8, 2024 08:29
@jsetoain jsetoain added this pull request to the merge queue May 8, 2024
Merged via the queue into Xilinx:main with commit 2b5a2fe May 8, 2024
54 checks passed
@jsetoain jsetoain deleted the move-ext-to-common-lowering branch May 8, 2024 09:14
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