[PATCH] D66295: [ARM] Sink add/mul(shufflevector(insertelement(...), ...), ...) for MVE instruction selection

[Sam Tebbs via Phabricator via llvm-commits]

samtebbs marked 2 inline comments as done.
samtebbs added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:14325
+      if (Shuffle) {
+        for (User* User : Shuffle->users()) {
+          Instruction* Insn = dyn_cast<Instruction>(User);
----------------
dmgreen wrote:
> Can you add a comment saying that we want all uses to be sunk (else we'll just end up with the same value in both gpr and vector regs!)
> 
> Also if this loop is moved to after the match check below, we won't need to do the O(n) loop if the O(1) match fails. Also negating the condition and returning early is apparently a good thing in llvm.
Agreed.


================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:14327
+          Instruction* Insn = dyn_cast<Instruction>(User);
+          if (!IsSinker(Insn, Op))
+            return false;
----------------
dmgreen wrote:
> I think this should be checking the operand of the use, which may not be the same for the original instruction.
The Op value here isn't actually used by the lambda and is only passed as a reference so that it can be set.


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

https://reviews.llvm.org/D66295