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

[Valeriy Dmitriev via Phabricator via llvm-commits]

vdmitrie 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;
----------------
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.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133430