[PATCH] [SLPVectorizer] Reorder operands of shufflevector if it can result in a vectorized code.

[Michael Zolotukhin]

Hi Karthik,

Thanks for updating the patch according to my numerous remarks:) To be honest, I think that the current path looks good, but I want to make sure we've considered all the possible cases before it gets in to the trunk. That'll guarantee that we do what we want to do in all the cases and the produced code is optimal.

Now back to the comments.

<hr class="remarkup-hr" />

> > 1. If the original operands formed a broadcast, do we want to touch them?

> 

> 

> No. As you mentioned if we detect a broadcast we do not reorder them.

>  We can return as soon as we detect a broadcast.




> > 5. What if left operands are broadcast, but if we swap one of them with a right one, we'll get consecutive access in the right operands?

> 

> 

> As you mentioned we don't want to disturb boardcast.

>  Hence we do not do anything here.


Great that we've agreed on this, it'll help to hoist the broadcast check and forget about it:)

<hr class="remarkup-hr" />

> > 2. What do we prefer: get AllSameOpcode operands, or operands, some of which are consecutive?

> 

> 

> If we have AllSameOpcode operands reodering it so that consecutive loads

>  are grouped together will not change AllSameOpcode property.

>  This case (i.e AllSameOpcode and code hits our swap logic) is only possible

>  when we have all loads in left and right side of the binary operation.)




> > 3. What if after swapping to create consecutive operands we lose AllSameOpcode property?

> 

> 

> This is not possible as far as i can tell.

>  If we have AllSameOpcode and if we enter in to the condition

>  to swap to create consecutive operands. It means that we have all loads in

>  left and right lane. Hence swaping will retain AllSameOpcode property

>  as we will still have loads on either side after swap.




> > 4. What if operands are AllSameOpcode, but not consecutive?

> 

> 

> Our logic should not alter anything in this scenario.


That's not exactly true. Consider the following example:

  b[0]     b[0]
  b[1]     a[0]+c[0]
  b[2]     a[1]*c[1]
  b[3]     d[5]/e[6]

Here we have `AllSameOpcodeLeft=false` and `AllSameOpcodeRight=true`. However, if we swap `b[1]` and  `a[0]+c[0]` (since `Right[0]` and `Left[1]` are consecutive), we'll lose a good candidate for vectorization.

This example not only shows, how we can lose `AllSameOpcode` property, but also how we can disturb initially good order by swapping operands.

And to reiterate: of course this is a constructed artificial case, and we most likely won't see anything like this in real tests. But such cases still should be considered since they can reveal flaws in our logic. I.e. it's fine if we can modify our logic to do the best in every case. But it's also fine if we decide to ignore such (hopefully rare) cases to keep the logic simple, but in this case I would prefer a comment explaining that it was an intentional, not just a random decision.

Thanks,
Michael


REPOSITORY
  rL LLVM

================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:1812-1813
@@ +1811,4 @@
+
+    Instruction *I0 = dyn_cast<Instruction>(V0);
+    Instruction *I1 = dyn_cast<Instruction>(V1);
+
----------------
Maybe it's better to rename I0 to ILeft, and I1 to IRight, or something like this? (the same for V0, P0 and others). What do you think?

http://reviews.llvm.org/D6677

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/