[PATCH] D133430: [SLP] Unify main/alternate instruction logic for CmpInst instructions - draft

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:582-586
-        auto *AltInst = cast<CmpInst>(VL[AltIndex]);
-        CmpInst::Predicate AltPred = AltInst->getPredicate();
-        if (BasePred == CurrentPred || BasePred == SwappedCurrentPred ||
-            AltPred == CurrentPred || AltPred == SwappedCurrentPred)
-          continue;
----------------
vdmitrie wrote:
> ABataev wrote:
> > Why did you removed this check? I believe it might help to build some nodes, though terminating with gathers.
> IMO, it is redundant ( i.e. removing it is no-op)
> For base instruction "IsMain == true" at line 580 means same as "(BasePred == CurrentPred || BasePred == SwappedCurrentPred) == true".
> Before setting the alternate op we don't really care about "AltPred == CurrentPred || AltPred == SwappedCurrentPred" as it the same as base.
> When we set alternate op (predicate) we don't care either as it is the first instruction with alternate  predicate. We do care only for subsequent instructions to fit either base or alternate flow.
> When alternate instruction is already set ("AltIndex != BaseIndex" is true) the check will be covered by "IsAlt == true" at line 585, which is  equivalent to "(AltPred == CurrentPred || AltPred == SwappedCurrentPred) == true".
> 
> Confusion probably comes from returning value of isCmpSameOrSwapped() routine.
> It returns optional which does only have a value when condition Pred == CurrentPredicate || Pred == SwappedCurrentPred is met.
> 
It will be covered if ops are compatible, if they are incompatible,  isCmpSameOrSwapped returns None and the functions returns `InstructionsState(VL[BaseIndex], nullptr, nullptr);`, saying that it is a gather sequence. But we can build a vector node even if there are no compatible operands, the operands will form the gather-based nodes instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133430