[llvm] [SLP][REVEC] Fix CommonMask is transformed into vector form but used outside finalize. (PR #120952)

[Alexey Bataev via llvm-commits]

alexey-bataev wrote:

> The current VF definition is based on the number of VL instead of the number of elements. If we modify the VF definition to the number of elements, it will require modifying a lot of code, and I don't see any clear benefits here. 

Modifying 'a lot of code' is not a problem here, correct cost estimation and vectorization factor is the actual problem. The benefit is that all possible combinations are processed in a uniform way, which correctly allows to estimate cost and emit code.

> For example, what is the VF you expect when we vectorize `FixedVectorType` in the following code?
> 
> ```
>   unsigned Sz = R.getVectorElementSize(I0);
>   unsigned MinVF = R.getMinVF(Sz);
>   unsigned MaxVF = std::max<unsigned>(llvm::bit_floor(VL.size()), MinVF);
>   MaxVF = std::min(R.getMaximumVF(Sz, S.getOpcode()), MaxVF);
> ```
> 
> I don't think partially modifying the definition in the `finalize` is a good idea either.
> 

Maybe, anyway you should operate on a full masks here. The user should not know if it operates on scalars or vectors, it just shall operate on the whole masks. Who expands these masks, the finalize function or other functions, is another question

> `ShuffleCostEstimator::createShuffle` can handle `FixedVectorType` now. It expands `CommonMask` into a correct mask before calling `BaseShuffleAnalysis::createShuffle`. We just need to make `GetNodeMinBWAffectedCost` and `GetValueMinBWAffectedCost` support `FixedVectorType`.
> 
> `ShuffleInstructionBuilder::finalize` can handle `FixedVectorType` now. Any development that modifies the mask can be easily identified and may cause a crash. At that point, we can quickly fix the bug.
> 
> I think the real solution is to wrap the mask into a class. This class can distinguish whether we are vectorizing a scalar to a vector or a vector to a vector. Additionally, we can add a cost model and an instruction builder within the class.

Bad decision. As I said, these classes should not know anything about high level abstraction, they should operate on actual masks/data


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