[PATCH] D38181: [x86] swap order of srl (and X, C1), C2 when it saves size

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:31785
+  // If the mask fits in a byte, then we know we can generate smaller and
+  // potentially better code by shifting first.
+  APInt MaskVal = AndC->getAPIntValue();
----------------
craig.topper wrote:
> What about a larger than 32-bit and mask that would allow us to use a 32-bit and? Otherwise we use a movabsq to load the immediate.
Yes, that's a limitation. I'll have to check if that causes regressions for the other patterns. Ok to make that a TODO in this patch?


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:31837
   if (N->getOpcode() == ISD::SRA)
     if (SDValue V = combineShiftRightAlgebraic(N, DAG))
       return V;
----------------
craig.topper wrote:
> Not related to this patch, but shouldn't that be "Arithmetic" not "Algebraic"?
'Algebraic' is the IBM / Power lingo:
https://www.ibm.com/support/knowledgecenter/en/ssw_aix_71/com.ibm.aix.alangref/idalangref_srawi_srai_instrs.htm
...which is probably why I chose it. I can make it 'Arithmetic' to be more x86.


================
Comment at: test/CodeGen/X86/urem-i8-constant.ll:10
 ; CHECK-NEXT:    shrl $12, %eax
+; CHECK-NEXT:    movzwl %ax, %eax
 ; CHECK-NEXT:    movb $37, %dl
----------------
craig.topper wrote:
> craig.topper wrote:
> > It's not immediately obvious to me how moving 0x7000 right by 12 bits turned into a mozwl.
> Oh there's magic in SelectionDAGISel::CheckAndMask that I never knew about.
Hmm...right, there's a very late computeKnownBits that I didn't see either. I did write a dirty test program to confirm that it's not miscompiling for any 8-bit urem (still waiting for Alive to come back).


https://reviews.llvm.org/D38181