[PATCH] D82961: [Scalarizer] Variable insert handling (PR46524)

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

In D82961#2128678 <https://reviews.llvm.org/D82961#2128678>, @arsenm wrote:

> In D82961#2128464 <https://reviews.llvm.org/D82961#2128464>, @jdoerfert wrote:
>
> > I like the flag idea, but I would expect this to be the default. If I read scalarizer and I enable it, I would expect the code to be scalarized, not partially scalarized based on a heuristic that may or may not be to my liking. The option to opt-out of some scalarization is sensible though.
>
>
> You have a different idea of what scalarization means than I do. I want to eliminate operations on vectors; I consider this as arithmetic operations operating on vectors. An insertelement/extractelement is not really a vector operation itself.


We might. My idea of scalarization is that it removes vector operations (= operations interacting with vector values). Put differently, I would expect all types/values to be scalars and not vectors after I run the scalarizer.



================
Comment at: llvm/lib/Transforms/Scalar/Scalarizer.cpp:759
+  if (isa<Constant>(InsIdx))
+    return false;
+
----------------
lebedev.ri wrote:
> bjope wrote:
> > jdoerfert wrote:
> > > I do not understand why we skip constant indices. 
> > I don't know exactly, but Scalarizer is sometimes creating such instructions itself when a vector still is needed.
> > 
> > (Downstream we have hacked the scalarizer to keep vector load/stores (typically loading/storing to a register pair/quad). But artithmetics etc needs to be scalarized. So there are typically lots of insertelement/extractelement instructions around to glue things together after running the scalarizer.)
> I mean, why shouldn't we skip them.
> I'm not sure replacing constant extract with n constant extracts+select is better.
Do we need n extracts+select for a constant index? wouldn't that fold into a single value?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82961