Skip to content

Commit 8341171

Browse files
committed
[mlir][Vector] Update VectorEmulateNarrowType.cpp
This PR aims at improving "VectorEmulateNarrowType.cpp". This is mainly minor refactoring, no major functional changes are made/added. Implements llvm#123630. **CHANGE 1** Renames `srcBits/dstBits` to `oldBits/newBits` to improve consistency in naming within the file. This is illustrated below: ```cpp // Extracted from VectorEmulateNarrowType.cpp Type oldElementType = op.getType().getElementType(); Type newElementType = convertedType.getElementType(); // BEFORE (mixing old/new and src/dst): // int srcBits = oldElementType.getIntOrFloatBitWidth(); // int dstBits = newElementType.getIntOrFloatBitWidth(); // AFTER (consistently using old/new): int oldBits = oldElementType.getIntOrFloatBitWidth(); int newBits = newElementType.getIntOrFloatBitWidth(); ``` Also adds some comments and unifies related "rewriter notification" messages. **CHANGE 2** Renames the variable "scale". Note, "scale" could mean either: * "original-elements-per-emulated-type", or * "emulated-elements-per-original-type". While from the context it is clear that it's always the former (original type is always a sub-byte type and the emulated type is usually `i8`), this PR reduces the cognitive load by making this clear. **CHANGE 3** Replaces `isUnalignedEmulation` with `isFullyAligned` Note, `isUnalignedEmulation` is always computed following a "per-element-alignment" condition: ```cpp // Check per-element alignment. if (newBits % oldBits != 0) { return rewriter.notifyMatchFailure(op, "unalagined element types"); } // (...) bool isUnalignedEmulation = origElements % elementsPerContainerType != 0; ``` Given that `isUnalignedEmulation` captures only one of two conditions required for "full alignment", it should be re-named as `isPartiallyUnalignedEmulation`. Instead, I've flipped the condition and renamed it as `isFullyAligned`: ```cpp bool isFullyAligned = origElements % elementsPerContainerType == 0; ``` **CHANGE 4** Unifies various comments throughout the file (for consistency). **CHANGE 5** Adds new comments throughout the file and adds TODOs where high-level comments are missing. **CHANGE 6** Update `alignedConversionPrecondition` (1): This method didn't require the vector type for the "destination" argument. The underlying element type is sufficient. The corresponding argument has been renamed as `multiByteScalarTy` - this is meant as the multi-byte emulated type (`i8`, `i16`, `i32`, etc). **CHANGE 7** Update `alignedConversionPrecondition` (2): In llvm#121298, we replaced `dstElemBitwidt` in this calculation: ```cpp const int numSrcElemsPerDestElem = dstElemBitwidth / srcElemBitwidth; ``` with the hard-coded value of 8: ```cpp const int numSrcElemsPerDestElem = 8 / srcElemBitwidth; ``` That was correct as for the patterns for which this hook was/is used: * `RewriteAlignedSubByteIntExt`, * `RewriteAlignedSubByteIntTrunc`. The destination type (or, more precisely, the emulated type) was always `i8`. In this PR, I am switching back to a more generic approach - the calculation should take into account the bit-width of the emulated type. Note that at the call sites I am passing `i8` as the emulated type, so the end-result is effectively identical. However, the intent is clearer, i.e., the underlying value is 8 because the emulated type happens to be `i8` (as opposed using a magic number). **CHANGE 8** Update alignedConversionPrecondition (3): The final check has been replaced with a new helper method, `isSubByteVecFittable`. This new method is also re-used within the code and hopefully will allow us more code re-use moving forward (to avoid re-implementing the same condition).
1 parent 3b001db commit 8341171

File tree

1 file changed

+234
-136
lines changed

1 file changed

+234
-136
lines changed

0 commit comments

Comments
 (0)