[PATCH] D132698: [SLP]Fix alternate cmp operands analysis.

[Valeriy Dmitriev via Phabricator via llvm-commits]

vdmitrie added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:3669
+  if (!TE.ReuseShuffleIndices.empty()) {
+    // Check if reuse shuffle indices can be improved by reordering.
+    // For this, check that reuse mask is "clustered", i.e. each scalar values
----------------
Is it possible to split off the reordering improvement into a separate patch?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:5883
     CmpInst::Predicate CurrentPred = CI->getPredicate();
     if (P0 == AltP0Swapped)
       return I == AltCI0 ||
----------------
The main issue with existing code here which makes me nervous is that main/alternate logic is not consistent across SLP. getSameOpcode has some clear enough approach but its logic does not match one here [and another similar code].
Unfortunately this patch does not fix that. That is maintainability issue too.
For isAlternateInstruction, for example, there are couple of check-points: it shall answer true if I == AltOp and false when I == MainOp. But it is very difficult to figure from this code whether it will work this way. 
Without clear description it is pretty difficult to reverse engineer the intent.
I put a draft patch here https://reviews.llvm.org/D133430 - as a possible approach to the problem.
It works very similar to this patch (not taking into account reordering improvement) but has consistent approach to main/alternate selection. There were couple of minor differences when an instruction could fit both main and alternate flows.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132698