This is beneficial when the shuffle is only used once and end up being generated in a few places when some node is combined into a shuffle.
Do you check that both vector_shuffle and extract_vector_elt have the same first argument?
Here we have two instructions, and only require that the first one is single-use.
Another observation - you end up keeping (using) the instruction you just checked for extra uses.
So NewMask is simply inited as a copy of SVN->getMask()?
You either need to check that the cast succeeded, or drop dyn_ prefix
This could use a comment
If i'm reading this correctly this does showcase the need to handle this in backend - it can be introduced there?
This is checked bellow. It is because we can match either the first or the second argument, as vector_shuffle does in fact shuffle two vectors together.
Even if InsertVal is reused, this transform reduces data dependencies, but indeed do not save anything on the instruction count from. Removing data dependencies is beneficial on its own.
I asked mys elf the same question. This doesn't seem to work.
The cast must succeed, as we check for it in the if guarding this code, so I'll remove dyn_
I'll add a few comments.
I've no objection to you creating a static helper that tries isShuffleMaskLegal with original and commuted masks, and call DAG.getVectorShuffle on success.
Actually, i have one concern.
Just x/y like in comments?
Actually, this is not correct.
There's no need to rename - tbh we don't know its a shuffle at this stage!
That is only true for IR - in the DAG, the result must have the same type as the shuffle operands - so the number of mask elts must match the number of result/operand elts - see SelectionDAG::getVectorShuffle
Please can you assert that IndexX's value (and InsIndex come to that) are both in bounds (0 <= idx < Mask.size())?
I still find naming choices to be actively misleading.
But we will only proceed *if* it will be a shuffle. This is a common pattern everywhere else in the code.