[PATCH] D59973: [SLP] Refactoring of the operand reordering code.

[Vasileios Porpodas via Phabricator via llvm-commits]

vporpo marked an inline comment as done.
vporpo added inline comments.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:991
+              // 'Failed'.
+              ReorderingModes[OpIdx] = ReorderingMode::Failed;
+          }
----------------
ABataev wrote:
> vporpo wrote:
> > ABataev wrote:
> > > vporpo wrote:
> > > > ABataev wrote:
> > > > > Shall we undo previously performed swaps in this case, if we could do some reordering earlier?
> > > > I don't think undoing the swaps would help much.  Following the reordering strategy for as long as we can without undoing on failure may actually help sometimes. For example, if our strategy is to look for the same opcode and half-way through the lanes we can no longer match it, we should still keep the order for the ones found so far, because we could potentially still vectorize these operands with a smaller vector length. If we undo them, that will not be possible.
> > > > 
> > > > Revisiting the failed operands and trying again with a different strategy could help. But I think we should try implement such back-tracking approaches in a separate optimization patch.
> > > Then, maybe, you should continue trying to reorder other elements in the same lane, and should not mark it as Failed too early? You can save the value somewhere in the temp array and update the original element to Failed state after the loop.
> > I am not sure I follow. I think we are not on the same page on how the ReorderingMode strategies work. Let me explain.
> > The ReorderingModes (e.g., Opcode or Failed)  are not per-lane, but per operand index (0 or 1 for now).
> > So even if an operand index is marked as "Failed" at some lane, the rest of the operands of that lane are still trying to reorder the elements.
> > The idea is that by marking an operand index as "Failed", we are announcing that it has the lowest priority in picking a value. This makes sense, because a "Failed" operand index is guaranteed not to form a full-length vector, but maybe other operands can still succeed, so they should have higher priority in selecting the operand values.
> > 
> > For example, operand 0 might be initialized with the "Opcode" strategy. Then, say that at lane 3 we can no longer find a matching opcode for operand 0, and operand 0 is therefore marked as "Failed". This means that from this point on, getBestOperand() will always return None for operand 0 on any lane, as operand 0 cannot possibly form a full-size vector. If operand 1 has not failed yet, it will be the one selecting the operand values, not operand 0. This is a good strategy, as operand 1may still succeed in forming a full-size vector.
> > 
> > Does this make sense?
> > 
> Yeah, but it may lead to some non-stable results. At first, we anounce that the operation has a high priority and reorder elements according to this high-pripority. Then, somewhere in the middle, we change to it to lower priority and change the reording. Is this really expected behavior? Or maybe it is better to make some preliminary analysis to identify the most successful strategy and only after that do the real reordering?
Ah yes, I agree. Ideally we should back-track and re-decide on the strategy, given that we failed half-way through. 
But if we don't back-track and we need to decide in one go, then I don't think there is anything really wrong with what we are doing now. I think it fully covers the functionality of the previous implementation, without adding complexity.


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

https://reviews.llvm.org/D59973