[PATCH] D125712: [SLP][X86] Improve reordering to consider alternate instruction bundles

[Valeriy Dmitriev via Phabricator via llvm-commits]

vdmitrie added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:2381
+    /// even and odd opcodes, like: OpcA, OpcB, OpcA, OpcB.
+    bool isAltShuffleWithRepeatingEvenOddOpcodes() const {
+      if (!isAltShuffle())
----------------
vporpo wrote:
> vdmitrie wrote:
> > I'd say we don't really need this interface.
> > IMO what is likely to be more useful is something like buildAltShuffleMask which you could directly feed into the TTI interface you added. See also buildShuffleEntryMask which could also benefit from having it.
> Well, the issue is that the TTI function can't check whether this is an actual add-sub pattern, because it doesn't have all the information needed. For this to work, it would have to know how we are planning to combine the scalar instructions into vectors, which is an SLP-specific thing. So it feels that this logic should not belong to TTI and instead it should all be in the SLP pass.
> 
> Regarding the mask, I don't really like the way this is done now. Actually I am not even sure passing the shuffle mask is a good idea because it doesn't actually help with checking the pattern. It is probably best to just pass the even/odd opcodes and the vector type and ask for TTI to very whether these the compatible ones with the addsub instructions.
> 
> Wdyt?
my 2 cents wrt the TTI interface
The interface as it defined in current revision has assumptions: two opcodes and that final result is built with select shuffle. And the latter is its main drawback. More generic version could look like
bool isLegalSIMDInstruction(Type* VecTy, ArrayRef<unsigned> Opcodes, ArrayRef<unsigned>LaneOpcMask)
where LaneOpcMask[Lane] is an index into Opcodes array which gives per lane opcode for a SIMD instruction.
Or if you want to limit it with just 2 opcodes (perhaps makes sense)  LaneOpcMask could be represented with a bit vector.




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125712/new/

https://reviews.llvm.org/D125712