[PATCH] D28907: [SLP] Fix for PR30787: Failure to beneficially vectorize 'copyable' elements in integer binary ops.

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added inline comments.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:1324
 
+    /// Reorder bundles from PseudoScheduleData data after scheduling,
+    /// if an Instruction is present in PseudoScheduleData that means this
----------------
dtemirbulatov wrote:
> ABataev wrote:
> > dtemirbulatov wrote:
> > > dtemirbulatov wrote:
> > > > ABataev wrote:
> > > > > dtemirbulatov wrote:
> > > > > > ABataev wrote:
> > > > > > > If you implement `InstructionOrPseudoOp` correctly, this reorder stuff should not be required, I think
> > > > > > well, There should be several(>=2) independent scheduling events(one for real instruction and other for pseudos) and there is just one real instruction, in the end, I don't see how it could be done without reordering or tracking the last scheduled instance for the same instruction. We could introduce something like IsLastScheduled field in ScheduleData struct, but it would be quite similar to reordering.
> > > > > If you add the real instruction instead of this pseudoinstruction, will you need all these scheduling events? No. Will you need to do some extra reordering etc.? No. Why you cannot simulate it with the new class/structure?
> > > > do you mean that the last one scheduling becomes the real one and we just ignore any pseudos?
> > > do you mean that the last one scheduled becomes the real one and we just ignore any pseudos?
> > No. I mean, that if you insert the real instructions instead of those pseudo-instructions, you won't need all that reordering/new scheduling etc. Why can't you mimic this behavior with the pseudo-instruction?
> hmm, There are at least NextLoadStore dependancies that we break if we, for example, insert real Load instruction somewhere. Or with could recalculate NextLoadStore. Or maybe mimic pseudo in some another way.
I don't think that they can be broken as we're not going to insert new Loads/Stores, just some binops. SO, the loads/stores and the the corresponding dependencies should not be affected.


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

https://reviews.llvm.org/D28907