[PATCH] D25913: [InstCombine] Fold nuw left-shifts in `ugt`/`ule` comparisons.

[Sanjay Patel via llvm-commits]

spatel added inline comments.


================
Comment at: test/Transforms/InstCombine/icmp-shl-zext-simplify.ll:37
+; CHECK-LABEL: @icmp_ule_i64x2(
+; CHECK-NEXT:    [[TMP2:%.*]] = trunc <2 x i64> %0 to <2 x i48>
+; CHECK-NEXT:    [[D:%.*]] = icmp eq <2 x i48> [[TMP2]], zeroinitializer
----------------
bryant wrote:
> spatel wrote:
> > bryant wrote:
> > > This `trunc` bothers me. On X86, I think it generates an extra `pand`:
> > > 
> > > ```
> > > .LCPI3_0:
> > >         .short  65535                   # 0xffff
> > >         .short  65535                   # 0xffff
> > >         .short  65535                   # 0xffff
> > >         .short  0                       # 0x0
> > >         .short  65535                   # 0xffff
> > >         .short  65535                   # 0xffff
> > >         .short  65535                   # 0xffff
> > >         .short  0                       # 0x0
> > >         .text
> > >         .globl  icmp_ule_i64x2
> > >         .p2align        4, 0x90
> > >         .type   icmp_ule_i64x2, at function
> > > icmp_ule_i64x2:                         # @icmp_ule_i64x2
> > >         .cfi_startproc
> > > # BB#0:
> > >         pand    .LCPI3_0(%rip), %xmm0      # <=======
> > >         pxor    %xmm1, %xmm1
> > >         pcmpeqd %xmm0, %xmm1
> > >         pshufd  $177, %xmm1, %xmm0      # xmm0 = xmm1[1,0,3,2]
> > >         pand    %xmm1, %xmm0
> > >         retq
> > > ```
> > > 
> > > The same goes for `icmp_ule_64`, but no extra X86 op is generated in that case.
> > I noticed that example too and agree it doesn't look like a good fold. We shouldn't be creating illegal types in InstCombine without a fallback in a later pass to make sure the backend is not harmed. 
> > 
> > But this is the trunc fold above yours that is firing, right? Ie, this test does not change with your patch.
> > Please add a vector test that is affected by your patch. 
> > 
> > You can add all of the tests with the current output based on trunk, then rebase this patch after that commit, so we'll see just the diffs in the resulting IR.
> I've already moved this patch above the trunc fold, though.
Your last update came in while I was writing my last comment, so I hadn't seen it.
Now there's even more reason to add the tests with current trunk output before the code patch because we should document the current behavior that your patch is going to alter. Please do that and rebase.


Repository:
  rL LLVM

https://reviews.llvm.org/D25913