[Mlir-commits] [llvm] [mlir] [mlir][vector] Prevent crashing on bitcast breakdown (PR #206683)

[llvmlistbot at llvm.org]

LouisLu060211 wrote:

> Hi @LouisLu060211,
> 
> Thank you for the patch! Unfortunately, I do not think Vector should move in this direction.
> 
> From your summary:
> 
> > For example, vector<16xi3> to vector<3xi16> has 48 bits on both sides, but 16 is not divisible by 3.
> 
> It sounds like you are looking for logic to model sub-byte types in Vector. There is already [VectorEmulateNarrowType](https://github.com/llvm/llvm-project/blob/0b5c006208cf7b0fee3709f468152e7de81607ef/mlir/lib/Dialect/Vector/Transforms/VectorEmulateNarrowType.cpp) for this purpose. While that pass works, it is admittedly somewhat fragile. In my opinion, this is because we never fully settled whether it should support arbitrary integer widths such as `i3`.
> 
> In hindsight, I think `VectorEmulateNarrowType` should not support such exotic types. The underlying logic is very complex, difficult to support, and there does not seem to be enough interest in maintaining it beyond very narrow use cases. In other words, the current support is not very general.
> 
> I also question whether `i3` is a type we should support beyond the most basic examples. I appreciate that some users may want this, but supporting it generally would come at a significant maintenance cost. IMHO, we should instead prioritise the more common cases that have more users and reviewers.
> 
> Is this a major blocker for you? If we start unblocking cases like this, I am worried that we will also need to review many other Vector transformations, such as unrolling and flattening, to make sure they bail out cleanly. That would be a substantial effort, and I would prefer to avoid it at this point.
> 
> If you have broader plans for sub-byte types, I would be happy to hear them.
> 
> Thank you!

@banach-space Thank you for the explanation.I found the crash through issue #206090, and I do not have broader plans for sub-byte types or a real use case for i3. I understand the maintenance concern. If the preferred direction is to reject these cases in the verifier instead, as joker-eph suggested, I am happy to rework the PR that way, or close this one if you would rather not go down that path.

> Hi @LouisLu060211,
> 
> Thank you for the patch! Unfortunately, I do not think Vector should move in this direction.
> 
> From your summary:
> 
> > For example, vector<16xi3> to vector<3xi16> has 48 bits on both sides, but 16 is not divisible by 3.
> 
> It sounds like you are looking for logic to model sub-byte types in Vector. There is already [VectorEmulateNarrowType](https://github.com/llvm/llvm-project/blob/0b5c006208cf7b0fee3709f468152e7de81607ef/mlir/lib/Dialect/Vector/Transforms/VectorEmulateNarrowType.cpp) for this purpose. While that pass works, it is admittedly somewhat fragile. In my opinion, this is because we never fully settled whether it should support arbitrary integer widths such as `i3`.
> 
> In hindsight, I think `VectorEmulateNarrowType` should not support such exotic types. The underlying logic is very complex, difficult to support, and there does not seem to be enough interest in maintaining it beyond very narrow use cases. In other words, the current support is not very general.
> 
> I also question whether `i3` is a type we should support beyond the most basic examples. I appreciate that some users may want this, but supporting it generally would come at a significant maintenance cost. IMHO, we should instead prioritise the more common cases that have more users and reviewers.
> 
> Is this a major blocker for you? If we start unblocking cases like this, I am worried that we will also need to review many other Vector transformations, such as unrolling and flattening, to make sure they bail out cleanly. That would be a substantial effort, and I would prefer to avoid it at this point.
> 
> If you have broader plans for sub-byte types, I would be happy to hear them.
> 
> Thank you!

@banach-space Thank you for the detailed explanation, and currently I do not have broader plans for sub-byte types or a real use case for i3. As I take a look into the VectorEmulateNarrowType, I do not think it will affect my fix toward this issue. Let me know if you find other concerns, and I am happy to take a look into it again.

https://github.com/llvm/llvm-project/pull/206683