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

Han-Kuan Chen via llvm-commits llvm-commits at lists.llvm.org
Thu Dec 26 23:34:54 PST 2024


HanKuanChen 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. 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.

`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.

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


More information about the llvm-commits mailing list