<div dir="ltr"><div>Thanks for looking closer - I wasn't even thinking about the flags difference.<br></div>Patch proposal coming up...<br></div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Aug 11, 2017 at 6:32 PM, Craig Topper <span dir="ltr"><<a href="mailto:craig.topper@gmail.com" target="_blank">craig.topper@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">I think we can do this based on what I believe is happening. I think based on by reading that hardware will mask it to 5-bits and it will rotate that many bits modulo the data size. This works fine for the data of the rol/ror. I think that the flags will be updated for 16 where for 0 they would not be touched. And I believe 17 will not have same flag behavior as 1 which is treated specially by the hardware. But I don't think we every assume anything about the flags from a variable rotate so that shouldn't matter.<div><br></div><div>I think if we were talking about RCR/RCL it would not work because it will rotate by the 5-bit value modulo data size plus 1. But we don't produce RCR/RCL so that's non-issue. But they are lumped together in the same section of Intel's documentation so felt I should mention.</div></div><div class="gmail_extra"><span class="HOEnZb"><font color="#888888"><br clear="all"><div><div class="m_7891622303245921507gmail_signature" data-smartmail="gmail_signature">~Craig</div></div></font></span><div><div class="h5">
<br><div class="gmail_quote">On Fri, Aug 11, 2017 at 3:52 PM, Sanjay Patel <span dir="ltr"><<a href="mailto:spatel@rotateright.com" target="_blank">spatel@rotateright.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>Yes, you're right. I even had the code commented out in that link. :)<br>I did test the output on Haswell with oversized rotate amounts, and it behaves like we would hope - rotating around again and/or masking off the high bit(s).<br></div>Is that enough to adjust the td patterns for these cases?<br></div><div class="m_7891622303245921507HOEnZb"><div class="m_7891622303245921507h5"><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Aug 11, 2017 at 4:45 PM, Craig Topper <span dir="ltr"><<a href="mailto:craig.topper@gmail.com" target="_blank">craig.topper@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">That's not really an existence proof. Isn't that just demonstrating that if you leave the mask out of the source code that gcc assumes that the shift has to be in bounds to avoid UB? If you put an explicit mask in the source code, gcc will remove it from 32 and 64 bit rotates, but not 16-bit rotates.<span class="m_7891622303245921507m_2749805576901523696HOEnZb"><font color="#888888"><div><br></div><div><br></div></font></span></div><div class="gmail_extra"><span class="m_7891622303245921507m_2749805576901523696HOEnZb"><font color="#888888"><br clear="all"><div><div class="m_7891622303245921507m_2749805576901523696m_-7704858325958568592gmail_signature" data-smartmail="gmail_signature">~Craig</div></div></font></span><div><div class="m_7891622303245921507m_2749805576901523696h5">
<br><div class="gmail_quote">On Fri, Aug 11, 2017 at 3:38 PM, Sanjay Patel via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: spatel<br>
Date: Fri Aug 11 15:38:40 2017<br>
New Revision: 310770<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=310770&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject?rev=310770&view=rev</a><br>
Log:<br>
[x86] add tests for rotate left/right with masked shifter; NFC<br>
<br>
As noted in the test comment, instcombine now produces the masked<br>
shift value even when it's not included in the source, so we should<br>
handle this.<br>
<br>
Although the AMD/Intel docs don't say it explicitly, over-rotating<br>
the narrow ops produces the same results. An existence proof that<br>
this works as expected on all x86 comes from gcc 4.9 or later:<br>
<a href="https://godbolt.org/g/K6rc1A" rel="noreferrer" target="_blank">https://godbolt.org/g/K6rc1A</a><br>
<br>
Modified:<br>
llvm/trunk/test/CodeGen/X86/ro<wbr>tate4.ll<br>
<br>
Modified: llvm/trunk/test/CodeGen/X86/ro<wbr>tate4.ll<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/X86/rotate4.ll?rev=310770&r1=310769&r2=310770&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject/llvm/trunk/test/CodeGen/<wbr>X86/rotate4.ll?rev=310770&r1=3<wbr>10769&r2=310770&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- llvm/trunk/test/CodeGen/X86/ro<wbr>tate4.ll (original)<br>
+++ llvm/trunk/test/CodeGen/X86/ro<wbr>tate4.ll Fri Aug 11 15:38:40 2017<br>
@@ -138,3 +138,154 @@ define void @rotate_right_m64(i64 *%pa,<br>
ret void<br>
}<br>
<br>
+; The next 8 tests include masks of the narrow width shift amounts that should be eliminated.<br>
+; These patterns are produced by instcombine after r310509.<br>
+<br>
+define i8 @rotate_left_8(i8 %x, i32 %amount) {<br>
+; CHECK-LABEL: rotate_left_8:<br>
+; CHECK: # BB#0:<br>
+; CHECK-NEXT: andb $7, %sil<br>
+; CHECK-NEXT: movl %esi, %ecx<br>
+; CHECK-NEXT: rolb %cl, %dil<br>
+; CHECK-NEXT: movl %edi, %eax<br>
+; CHECK-NEXT: retq<br>
+ %amt = trunc i32 %amount to i8<br>
+ %sub = sub i8 0, %amt<br>
+ %maskamt = and i8 %amt, 7<br>
+ %masksub = and i8 %sub, 7<br>
+ %shl = shl i8 %x, %maskamt<br>
+ %shr = lshr i8 %x, %masksub<br>
+ %or = or i8 %shl, %shr<br>
+ ret i8 %or<br>
+}<br>
+<br>
+define i8 @rotate_right_8(i8 %x, i32 %amount) {<br>
+; CHECK-LABEL: rotate_right_8:<br>
+; CHECK: # BB#0:<br>
+; CHECK-NEXT: andb $7, %sil<br>
+; CHECK-NEXT: movl %esi, %ecx<br>
+; CHECK-NEXT: rorb %cl, %dil<br>
+; CHECK-NEXT: movl %edi, %eax<br>
+; CHECK-NEXT: retq<br>
+ %amt = trunc i32 %amount to i8<br>
+ %sub = sub i8 0, %amt<br>
+ %maskamt = and i8 %amt, 7<br>
+ %masksub = and i8 %sub, 7<br>
+ %shr = lshr i8 %x, %maskamt<br>
+ %shl = shl i8 %x, %masksub<br>
+ %or = or i8 %shr, %shl<br>
+ ret i8 %or<br>
+}<br>
+<br>
+define i16 @rotate_left_16(i16 %x, i32 %amount) {<br>
+; CHECK-LABEL: rotate_left_16:<br>
+; CHECK: # BB#0:<br>
+; CHECK-NEXT: andb $15, %sil<br>
+; CHECK-NEXT: movl %esi, %ecx<br>
+; CHECK-NEXT: rolw %cl, %di<br>
+; CHECK-NEXT: movl %edi, %eax<br>
+; CHECK-NEXT: retq<br>
+ %amt = trunc i32 %amount to i16<br>
+ %sub = sub i16 0, %amt<br>
+ %maskamt = and i16 %amt, 15<br>
+ %masksub = and i16 %sub, 15<br>
+ %shl = shl i16 %x, %maskamt<br>
+ %shr = lshr i16 %x, %masksub<br>
+ %or = or i16 %shl, %shr<br>
+ ret i16 %or<br>
+}<br>
+<br>
+define i16 @rotate_right_16(i16 %x, i32 %amount) {<br>
+; CHECK-LABEL: rotate_right_16:<br>
+; CHECK: # BB#0:<br>
+; CHECK-NEXT: andb $15, %sil<br>
+; CHECK-NEXT: movl %esi, %ecx<br>
+; CHECK-NEXT: rorw %cl, %di<br>
+; CHECK-NEXT: movl %edi, %eax<br>
+; CHECK-NEXT: retq<br>
+ %amt = trunc i32 %amount to i16<br>
+ %sub = sub i16 0, %amt<br>
+ %maskamt = and i16 %amt, 15<br>
+ %masksub = and i16 %sub, 15<br>
+ %shr = lshr i16 %x, %maskamt<br>
+ %shl = shl i16 %x, %masksub<br>
+ %or = or i16 %shr, %shl<br>
+ ret i16 %or<br>
+}<br>
+<br>
+define void @rotate_left_m8(i8* %p, i32 %amount) {<br>
+; CHECK-LABEL: rotate_left_m8:<br>
+; CHECK: # BB#0:<br>
+; CHECK-NEXT: andb $7, %sil<br>
+; CHECK-NEXT: movl %esi, %ecx<br>
+; CHECK-NEXT: rolb %cl, (%rdi)<br>
+; CHECK-NEXT: retq<br>
+ %x = load i8, i8* %p, align 1<br>
+ %amt = trunc i32 %amount to i8<br>
+ %sub = sub i8 0, %amt<br>
+ %maskamt = and i8 %amt, 7<br>
+ %masksub = and i8 %sub, 7<br>
+ %shl = shl i8 %x, %maskamt<br>
+ %shr = lshr i8 %x, %masksub<br>
+ %or = or i8 %shl, %shr<br>
+ store i8 %or, i8* %p, align 1<br>
+ ret void<br>
+}<br>
+<br>
+define void @rotate_right_m8(i8* %p, i32 %amount) {<br>
+; CHECK-LABEL: rotate_right_m8:<br>
+; CHECK: # BB#0:<br>
+; CHECK-NEXT: andb $7, %sil<br>
+; CHECK-NEXT: movl %esi, %ecx<br>
+; CHECK-NEXT: rorb %cl, (%rdi)<br>
+; CHECK-NEXT: retq<br>
+ %x = load i8, i8* %p, align 1<br>
+ %amt = trunc i32 %amount to i8<br>
+ %sub = sub i8 0, %amt<br>
+ %maskamt = and i8 %amt, 7<br>
+ %masksub = and i8 %sub, 7<br>
+ %shl = shl i8 %x, %masksub<br>
+ %shr = lshr i8 %x, %maskamt<br>
+ %or = or i8 %shl, %shr<br>
+ store i8 %or, i8* %p, align 1<br>
+ ret void<br>
+}<br>
+<br>
+define void @rotate_left_m16(i16* %p, i32 %amount) {<br>
+; CHECK-LABEL: rotate_left_m16:<br>
+; CHECK: # BB#0:<br>
+; CHECK-NEXT: andb $15, %sil<br>
+; CHECK-NEXT: movl %esi, %ecx<br>
+; CHECK-NEXT: rolw %cl, (%rdi)<br>
+; CHECK-NEXT: retq<br>
+ %x = load i16, i16* %p, align 1<br>
+ %amt = trunc i32 %amount to i16<br>
+ %sub = sub i16 0, %amt<br>
+ %maskamt = and i16 %amt, 15<br>
+ %masksub = and i16 %sub, 15<br>
+ %shl = shl i16 %x, %maskamt<br>
+ %shr = lshr i16 %x, %masksub<br>
+ %or = or i16 %shl, %shr<br>
+ store i16 %or, i16* %p, align 1<br>
+ ret void<br>
+}<br>
+<br>
+define void @rotate_right_m16(i16* %p, i32 %amount) {<br>
+; CHECK-LABEL: rotate_right_m16:<br>
+; CHECK: # BB#0:<br>
+; CHECK-NEXT: andb $15, %sil<br>
+; CHECK-NEXT: movl %esi, %ecx<br>
+; CHECK-NEXT: rorw %cl, (%rdi)<br>
+; CHECK-NEXT: retq<br>
+ %x = load i16, i16* %p, align 1<br>
+ %amt = trunc i32 %amount to i16<br>
+ %sub = sub i16 0, %amt<br>
+ %maskamt = and i16 %amt, 15<br>
+ %masksub = and i16 %sub, 15<br>
+ %shl = shl i16 %x, %masksub<br>
+ %shr = lshr i16 %x, %maskamt<br>
+ %or = or i16 %shl, %shr<br>
+ store i16 %or, i16* %p, align 1<br>
+ ret void<br>
+}<br>
+<br>
<br>
<br>
______________________________<wbr>_________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/llvm-commits</a><br>
</blockquote></div><br></div></div></div>
</blockquote></div><br></div>
</div></div></blockquote></div><br></div></div></div>
</blockquote></div><br></div>