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

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added a comment.

I should say something, as we use the pass downstream (we run Scalarizer early in llc pipeline, partly for historical reasons, but still due to sometimes getting better code when scalarizing before some other pass (maybe LoopStrengthReduce/CodeGenPrepare?) instead of doing it at ISel).

I think the normal expand at ISel is doing a simple, store vector to stack, do an indexed store, load back the vector. So one could get condition/branch free code (except a check that index is within range). If such a lowering is good/bad for a target will ofcourse depend on if load/store is fast etc (might depend on number of elements in the vector as well). Basically it is easier to deal with the vector version and convert it to whatever is suitable for the target at ISel (rather than trying to convert the scalarized sequence into something using the indexed write technique). No idea if that is the intent with not rewriting insertelement/extractelement with variable index. But this patch might hurt someone out there that is expecting such lowering.

With the above said:

- I've checked our downstream lowering (at ISel) and we do lower the insertelement using the standard expand technique (we get a vector store, an indexed store for the element and a vector load). With this patch we get code with lots of conditional moves. However, in reality these insertelements with variable index probably never happens so it doesn't really matter what we do.
- With D82970 <https://reviews.llvm.org/D82970> in mind, our downstream lowering (at ISel) is expanding an extractelement by using the conditional moves. So that patch has even less impact for us.
- The pass is supposed to remove vector ops, so I don't think it is essentially wrong to do these new rewrites. However, it still using insertelement/extractelement with constant indices so it is not obvious that some insert/extracts should be scalarized and others not.
- I don't think it hurts to add an option that could toggle these new scalarizations on/off (in case someone runs into trouble with this, considering that the pass hasn't scalarized these in the past . Then at least, someone downstream could easily benchmark what happens when toggling this option. Or even override the default somehow if that is desired.

No expert on poison etc, but I figure the resulting code is less poisonous (insertelement can generate poison, but the select sequence won't). I guess that is ok.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82961