| Issue |
123630
|
| Summary |
[mlir] Improve code re-use in VectorEmulateNarrowType.cpp
|
| Labels |
|
| Assignees |
|
| Reporter |
banach-space
|
Hi folks!
Having reviewed a few recent PRs for [VectorEmulateNarrowType.cpp](https://github.com/llvm/llvm-project/blob/main/mlir/lib/Dialect/Vector/Transforms/VectorEmulateNarrowType.cpp), e.g.:
* https://github.com/llvm/llvm-project/pull/115922, or
* #121298,
I believe it’s time to improve code reusability and unify naming conventions in the file. At ~1.7k lines, it's becoming increasingly difficult to review patches.
### Proposal 1
To begin with, I would like for us to start re-using the logic from [alignedConversionPrecondition](https://github.com/llvm/llvm-project/blob/3b001db4f9668cfa29572e5f1911ec7cef8b0ac2/mlir/lib/Dialect/Vector/Transforms/VectorEmulateNarrowType.cpp#L1084-L1109). This hook effectively checks **two conditions**:
1. [Per-element alignment](https://github.com/llvm/llvm-project/blob/3b001db4f9668cfa29572e5f1911ec7cef8b0ac2/mlir/lib/Dialect/Vector/Transforms/VectorEmulateNarrowType.cpp#L1096) (e.g. `i2` and `i8` are aligned, `i3` and `i8` are not aligned).
2. Could the input/source _vector of sub-byte scalar types_ (e.g. `vector<2xi4>`) fit within the target mulit-byte container type (e.g. `i8` or `i32`), this is checked [here](https://github.com/llvm/llvm-project/blob/3b001db4f9668cfa29572e5f1911ec7cef8b0ac2/mlir/lib/Dialect/Vector/Transforms/VectorEmulateNarrowType.cpp#L1103).
Similar checks are repeated throughout the code, e.g.
* **Cond 1:** [check 1](https://github.com/llvm/llvm-project/blob/3b001db4f9668cfa29572e5f1911ec7cef8b0ac2/mlir/lib/Dialect/Vector/Transforms/VectorEmulateNarrowType.cpp#L320), [check 2](https://github.com/llvm/llvm-project/blob/3b001db4f9668cfa29572e5f1911ec7cef8b0ac2/mlir/lib/Dialect/Vector/Transforms/VectorEmulateNarrowType.cpp#L391), [check 3](https://github.com/llvm/llvm-project/blob/3b001db4f9668cfa29572e5f1911ec7cef8b0ac2/mlir/lib/Dialect/Vector/Transforms/VectorEmulateNarrowType.cpp#L499) ... .
* **Cond 2:** [check 1](https://github.com/llvm/llvm-project/blob/3b001db4f9668cfa29572e5f1911ec7cef8b0ac2/mlir/lib/Dialect/Vector/Transforms/VectorEmulateNarrowType.cpp#L340), [check 2](https://github.com/llvm/llvm-project/blob/3b001db4f9668cfa29572e5f1911ec7cef8b0ac2/mlir/lib/Dialect/Vector/Transforms/VectorEmulateNarrowType.cpp#L398), [check 3](https://github.com/llvm/llvm-project/blob/3b001db4f9668cfa29572e5f1911ec7cef8b0ac2/mlir/lib/Dialect/Vector/Transforms/VectorEmulateNarrowType.cpp#L535), ... .
In order to enable re-use, it would help to do some minor variable renaming first. Otherwise, it's quite hard to see that basically identical quantity is computed/defined multiple times throughout the code. Rather than implementing this in one big patch, I have split it into 4 independent changes:
* https://github.com/llvm/llvm-project/pull/123526
* https://github.com/llvm/llvm-project/pull/123527
* https://github.com/llvm/llvm-project/pull/123528
* https://github.com/llvm/llvm-project/pull/123529
These are basically stacked PRs. I've made sure to link the relevant commit (1 per PR) in the summary, so that it's easy for you to find it.
### Proposal 2
We should clarify the meaning of "source" and "destination" types. Currently, the usage is ambiguous. For example, for this `arith.extsi` Op,
```mlir
%0 = arith.extsi %arg0 : vector<8xi2> to vector<8xi32>
```
`vector<8xi2>` and `vector<8xi32>` are the "source" and "destination" types, respectively (i.e. that's the interpretation we tend to use).
However, patterns like `RewriteAlignedSubByteIntExt` introduce `vector.bitcast` Ops like this:
```mlir
%bitcast = vector.bitcast %arg0 : vector<8xi2> to vector<2xi8>
```
I've noticed that we tend to consider both:
* `vector<2xi8>`(the result of `vector.bitcast`), and `vector<8xi32>` (the output of `arith.extsi`)
as the destination type. That should be clarified. I suggest two steps:
* Avoid "destination" and "source" when referring to the "emulation" logic. Reserve that for the original Op (e.g. `arith.extsi` above). Better still, use the naming from Op definitions (for [arith.extsi](https://mlir.llvm.org/docs/Dialects/ArithOps/#arithextsi-arithextsiop) that would mean `in` and `out` for "source" and "destination").
* For the "emulation" logic, use "emulated type" and "container name" for the original type (to be emulated, e.g. `i2`) and the target type that's used for emulation (e.g. `i8`).
WDYT?
### Proposal 3
Document the updated naming in the file.
![]()
_______________________________________________
llvm-bugs mailing list
llvm-bugs@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-bugs
