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

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n marked an inline comment as not done.
goldstein.w.n 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.
----------------
craig.topper wrote:
> 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.
> How many entries are there that do something for non-constant in the current patch?
> 
At the moment only have 11 cases actually on. The rest are placaeholder for when backend gets improved.

> 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.

Agreed, thats why most are off at the moment. I am working on a patch to improve vpternlog generation at which point we will hopefully be able to turn more on w.o causing regressions.

Think ternlogd is a bit simpler. Unlike shuffles the pattern for creating them is pretty simple. Take an arbitrary number of logic ops that use the same 3 operands -> ternlog. The issue we have now is we only look at 3-ops instead of looking at 3-operands.

But OFC will test results and won't turn on cases that still cause regressions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145220