[PATCH] D46179: [X86] Lowering adds/addus/subs/subus intrinsics to native IR (LLVM part)

Roland Scheidegger via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu May 17 12:04:30 PDT 2018


sroland added inline comments.


================
Comment at: test/CodeGen/X86/avx2-intrinsics-fast-isel.ll:2585-2587
+; CHECK-NEXT:    vpmaxub %ymm1, %ymm0, %ymm0
+; CHECK-NEXT:    vpsubb %ymm1, %ymm0, %ymm0
 ; CHECK-NEXT:    ret{{[l|q]}}
----------------
tkrupa wrote:
> sroland wrote:
> > tkrupa wrote:
> > > sroland wrote:
> > > > tkrupa wrote:
> > > > > sroland wrote:
> > > > > > Err, so it doesn't actually recognize the pattern?
> > > > > Please look at my comment from Thu, May 10, 10:27 AM.
> > > > Ah sorry I missed that.
> > > > I'm not quite sure if that means the pattern can never be folded into a single instruction, but if so I'm very much against autoupgrade. The code might not be much worse, but in 99.99% of all cases it's going to be worse than using the intrinsic.
> > > It only fails to fold with fast-isel (one of the non-standard instruction selection options chosen with a compiler flag); it is still always being combined with default SelectionDAGISel.
> > Ok, we're not using that but I believe at some point we considered it (compile times are really important with jit, and that's my other problem with autoupgrade anyway, this is not going to help there but it might not be significant and I'm thinking that's just the price you have to pay for the compiler getting smarter).
> Can fast-isel case be left like that then? If not, I could try to do the MachineCombiner folding (we don't currently have any combining like that for x86 from what I've seen) or abandon doing the AutoUpgrade for this particular intrinsic.
>From my perspective for mesa, yes, it should be ok.


Repository:
  rL LLVM

https://reviews.llvm.org/D46179





More information about the llvm-commits mailing list