[PATCH] D13851: [X86][XOP] Add support for lowering vector rotations

[Elena Demikhovsky via llvm-commits]

delena added a comment.

> > The rotation immediate can be negative. It will mean rotation right. I assume that the combiner catches this scenario and generates ROTL with negative value. Could you, please, add a test for this?

> 

> 

> I've tried to create such a test but without success - similar to shifts, for the ISD rotate ops any value less than 0 or greater than/equal to bitsize is undefined so I consider this to be correct for the XOP lowering. I've already removed the unnecessary XOP lowering of ISD::ROTR.

> 

> Would updating the lowering assert to check that the (sign extended rotation) constants are in range be satisfactory instead? Failing that I would have to add a ((1 << bitsize) - 1) mask to force the rotation to be in range which I believe isn't necessary in the same way that we don't do this for AVX2/XOP shifts by variable.


I ran a scalar test with debugger to see what happens. I modified the DAG combiner and  set ISD::ROTR to illegal. So I worked only with ROTL. Now I see that immediate is always positive.
Because "rotr $25" and "rotl $7" for 32 bit elt are the same.
Now I tried this test case:

define i32 @rotate_right_32(i32 %a, i32 %b) {
entry:

  %and = and i32 %b, 31
  %shl = lshr i32 %a, %and
  %0 = sub i32 0, %b
  %and3 = and i32 %0, 31
  %shr = shl i32 %a, %and3
  %or = or i32 %shl, %shr
  ret i32 %or

}
With disabled ROTR I received one instruction more:

  negl    %esi
  movb    %sil, %cl
  roll    %cl, %edi
  movl    %edi, %eax
  retq

And this code when we have the both ROTR and ROTL

  movb    %sil, %cl
  rorl    %cl, %edi
  movl    %edi, %eax
  retq

I'm almost sure that your code is correct. Could you, please, check the vector version of the test above? You should see the "negl" operation, probably in "sub" form.
And if you see that the code is correct, you can commit.
Thank you.


Repository:
  rL LLVM

http://reviews.llvm.org/D13851