[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:
> vdmitrie wrote:
> > ABataev wrote:
> > > 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.
> > I see what you mean. we make a call to areCompatibleCmpOps and then simply ignore it here. Right?
> > I'd instead tweaked areCompatibleCmpOps routine if we really miss something useful from that.
> > Returning the condition will bring inconsistency back. We have to be definite from very beginning about which scalars go to "base" and which to "alternate" and it should have deterministic behavior.
> Thinking of another possible alternative:  current behavior should be retained without sacrificing consistency. We can make isCmpSameOrSwapped  to return an optional enum instead:
> 
> MatchWithOperands               -  corresponds to current "true"
> MatchWithOperandsSwap      -  corresponds to current "false"
> PredicateMatch
> SwappedPredicateMatch
> 
> in buildTree_rec () we then also check for SwappedPredicateMatch in order to swap LHS/RHS.
> 
> 
> 
> 
You don’t need to make it optimal then, just add another enum value. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133430