[PATCH] D145220: [X86][InstCombine] Simplify some `pternlog` intrinsics

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/X86/X86InstCombineIntrinsic.cpp:591-594
+  // Currently we only handle cases that convert directly to another instruction
+  // or cases where all the ops are constant.  This is because we don't properly
+  // handle creating ternary ops in the backend, so splitting them here may
+  // cause regressions. As the backend improves, uncomment more cases.
----------------
goldstein.w.n wrote:
> goldstein.w.n wrote:
> > pengfei wrote:
> > > Maybe better to do it in FE and remove all these intrinsics finally?
> > What do you mean? Like in the header for `_mm_ternarylogic_....` add a switch statement? Or something else?
> > 
> > But would prefer to keep it somewhere we have const/non-const info on A/B/C because it will currently cause regressions to simplify many of the cases (working on a patch for backend generation, but a ways away).
> > What do you mean? Like in the header for `_mm_ternarylogic_....` add a switch statement? Or something else?
> > 
> > But would prefer to keep it somewhere we have const/non-const info on A/B/C because it will currently cause regressions to simplify many of the cases (working on a patch for backend generation, but a ways away).
> 
> We can handle the constants algorithimically if you prefer (my feeling however is 1 implementation is preferred and if we need the table would be simpler to just use that).
How many entries are there that do something for non-constant in the current patch?

I'm bit nervous about replacing vpternlog intrinsics with complex sequences and hoping to turn it back in the backend. If there are many logic ops involved we might make a different decision than the programmer. We already get bugs from time to time that we changed the shuffles that a programmer wrote and generated worse code.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145220/new/

https://reviews.llvm.org/D145220