[mlir][ArmSME] Use ArmSMETypeConverter for all VectorToLLVM patterns #65261
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
LLVMTypeConverter::convertVectorType asserts on n-D scalable vectors to prevent generating illegal LLVM IR, since LLVM doesn't support arrays of scalable vectors. The ArmSMETypeConverter disables this conversion, but is only used for ArmSME dialect conversions that rewrite higher-level custom ArmSME ops to intrinsics. This is problematic if we want to lower Vector ops directly to ArmSME intrinsics, as the assert fires for ops that have dialect conversion patterns (defined in ConvertVectorToLLVMPass, e.g. populateVectorToLLVMConversionPatterns) that use the LLVMTypeConverter. There are three options to get around this: 1. Avoid the generic VectorToLLVM dialect conversion patterns (and thus the assert) altogether by first lowering Vector ops to custom ArmSME ops. 2. Disable the generic VectorToLLVM dialect conversion patterns if ArmSME is enabled. 3. Disable n-D scalable vector type conversion for all dialect conversion patterns if SME is enabled. Option 1 is already done for several Vector ops such as vector.load and vector.store as part of ConvertVectorToArmSME, but where possible we'd like to avoid bloating the ArmSME dialect by having to mirror all the Vector ops. Option 2 is undesirable as the generic conversions should only be disabled for the 2-d scalable vector types the ArmSME patterns apply to. We'd still like Vector ops with other types to get lowered via the default path when ArmSME is enabled. This patch therefore implements option 3 to use the ArmSMETypeConverter for all VectorToLLVM conversion patterns when ArmSME is enabled.
zero9178
reviewed
Sep 4, 2023
Comment on lines
+87
to
+91
| LLVMTypeConverter *converter; | ||
| if (armSME) | ||
| converter = new arm_sme::ArmSMETypeConverter(&getContext(), options); | ||
| else | ||
| converter = new LLVMTypeConverter(&getContext(), options); |
There was a problem hiding this comment.
This is a memory leak. You should at the very least use std::unique_ptr<LLVMTypeConverter> here and use make_unique in the logic.
There was a problem hiding this comment.
ah, thanks for heads up! I'll push a fix soon
c-rhodes
added a commit
to c-rhodes/llvm-project
that referenced
this pull request
Sep 6, 2023
This patch changes vector type conversion to return failure on n-D scalable vector types instead of asserting. This is an alternative approach to llvm#65261 that aims to enable lowering of Vector ops directly to ArmSME intrinsics where possible, and seems more consistent with other type conversions. It's trivial to hit the assert at the moment and it could be interpreted as n-D scalable vector types being a bug, when they're valid types in the Vector dialect. By returning failure it will generally fail more gracefully, particularly for release builds or other builds where assertions are disabled.
|
I've created a pull request #65450 with an alternative approach to return failure from the LLVM type converter for n-D scalable vectors instead of asserting. |
|
I'm trying to get a sense of the current state... Aren't arrays of scalable vectors in LLVM on their way to be supported? That should give us a option 4... I'm worried about the current approach as not all the operations with vector types are converted in VectorToLLVM (e.g., Arith, Index...). Thoughts? |
Only (non-scalable) arrays of scalable vectors. So only a single trailing scalable dimension would be supported in LLVM (so it still can't represent SME tiles). |
c-rhodes
added a commit
to c-rhodes/llvm-project
that referenced
this pull request
Sep 7, 2023
This patch changes vector type conversion to return failure on n-D scalable vector types instead of asserting. This is an alternative approach to llvm#65261 that aims to enable lowering of Vector ops directly to ArmSME intrinsics where possible, and seems more consistent with other type conversions. It's trivial to hit the assert at the moment and it could be interpreted as n-D scalable vector types being a bug, when they're valid types in the Vector dialect. By returning failure it will generally fail more gracefully, particularly for release builds or other builds where assertions are disabled.
|
Abandoning this in favour of #65450 |
c-rhodes
added a commit
that referenced
this pull request
Sep 11, 2023
…ors (#65450) This patch changes vector type conversion to return failure on n-D scalable vector types instead of asserting. This is an alternative approach to #65261 that aims to enable lowering of Vector ops directly to ArmSME intrinsics where possible, and seems more consistent with other type conversions. It's trivial to hit the assert at the moment and it could be interpreted as n-D scalable vector types being a bug, when they're valid types in the Vector dialect. By returning failure it will generally fail more gracefully, particularly for release builds or other builds where assertions are disabled.
ZijunZhaoCCK
pushed a commit
to ZijunZhaoCCK/llvm-project
that referenced
this pull request
Sep 19, 2023
…ors (llvm#65450) This patch changes vector type conversion to return failure on n-D scalable vector types instead of asserting. This is an alternative approach to llvm#65261 that aims to enable lowering of Vector ops directly to ArmSME intrinsics where possible, and seems more consistent with other type conversions. It's trivial to hit the assert at the moment and it could be interpreted as n-D scalable vector types being a bug, when they're valid types in the Vector dialect. By returning failure it will generally fail more gracefully, particularly for release builds or other builds where assertions are disabled.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
LLVMTypeConverter::convertVectorType asserts on n-D scalable vectors to prevent generating illegal LLVM IR, since LLVM doesn't support arrays of scalable vectors. The ArmSMETypeConverter disables this conversion, but is only used for ArmSME dialect conversions that rewrite higher-level custom ArmSME ops to intrinsics.
This is problematic if we want to lower Vector ops directly to ArmSME intrinsics, as the assert fires for ops that have dialect conversion patterns (defined in ConvertVectorToLLVMPass, e.g. populateVectorToLLVMConversionPatterns) that use the LLVMTypeConverter.
There are three options to get around this:
Option 1 is already done for several Vector ops such as vector.load and vector.store as part of ConvertVectorToArmSME, but where possible we'd like to avoid bloating the ArmSME dialect by having to mirror all the Vector ops.
Option 2 is undesirable as the generic conversions should only be disabled for the 2-d scalable vector types the ArmSME patterns apply to. We'd still like Vector ops with other types to get lowered via the default path when ArmSME is enabled.
This patch therefore implements option 3 to use the ArmSMETypeConverter for all VectorToLLVM conversion patterns when ArmSME is enabled.
Depends on #65254