[PATCH] D78728: [ARM] Convert floating point splats to integer

Tue May 12 05:20:37 PDT 2020

spatel added inline comments.

================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:6436
 /// codegen can spot all lanes are identical.
-bool CodeGenPrepare::optimizeShuffleVectorInst(ShuffleVectorInst *SVI) {
+bool CodeGenPrepare::sinkShuffleVectorToShift(ShuffleVectorInst *SVI) {
   BasicBlock *DefBB = SVI->getParent();
----------------
dmgreen wrote:
> efriedma wrote:
> > I guess this is only loosely related to your patch, but should this transform also be handled by tryToSinkFreeOperands?
> Yeah, I think it pre-dates the tryToSinkFreeOperands version but only handles the shifts that X86 cares about. I can have a look at changing it over to see if it can use SinkFreeOperands, but @spatel is updating it in D79718. I'll at least wait until that has been done.
Thanks for cc'ing me. This function seems worse than just awkwardly structured; it can do transforms that were not intended. That's because we are walking the users of the splat shuffle without checking whether the actual use is as a shift's amount operand (operand 1 of a regular shift opcode). That could be a (hopefully minor) perf bug if it ever happens, but that's probably still not what we want to happen.

I think this transform should begin from the shift instruction, not the shuffle. tryToSinkFreeOperands() looks promising to replace this and the TLI hook. I'll take a look at updating this now, but that doesn't need to block this patch.

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

https://reviews.llvm.org/D78728