[llvm] [GlobalISel] Turn shuffle a, b, mask -> shuffle undef, b, mask iff mask does not reference a (PR #115377)

[Amara Emerson via llvm-commits]

================
@@ -7726,3 +7726,65 @@ bool CombinerHelper::matchShuffleUndefRHS(MachineInstr &MI,
 
   return true;
 }
+
+static void commuteMask(MutableArrayRef<int> Mask, const unsigned NumElems) {
+  const unsigned MaskSize = Mask.size();
+  for (unsigned I = 0; I < MaskSize; ++I) {
+    int Idx = Mask[I];
+    if (Idx < 0)
+      continue;
+
+    if (Idx < (int)NumElems)
+      Mask[I] = Idx + NumElems;
+    else
+      Mask[I] = Idx - NumElems;
+  }
+}
+
+bool CombinerHelper::matchShuffleDisjointMask(MachineInstr &MI,
+                                              BuildFnTy &MatchInfo) {
+
+  auto &Shuffle = cast<GShuffleVector>(MI);
+  // If any of the two inputs is already undef, don't check the mask again to
+  // prevent infinite loop
+  if (getOpcodeDef(TargetOpcode::G_IMPLICIT_DEF, Shuffle.getSrc1Reg(), MRI))
+    return false;
+
+  if (getOpcodeDef(TargetOpcode::G_IMPLICIT_DEF, Shuffle.getSrc2Reg(), MRI))
+    return false;
+
+  ArrayRef<int> Mask = Shuffle.getMask();
+  const LLT Src1Ty = MRI.getType(Shuffle.getSrc1Reg());
+
+  const unsigned NumSrcElems = Src1Ty.isVector() ? Src1Ty.getNumElements() : 1;
+
+  bool TouchesSrc1 = false;
+  bool TouchesSrc2 = false;
+  const unsigned NumElems = Mask.size();
+  for (unsigned Idx = 0; Idx < NumElems; ++Idx) {
+    if (Mask[Idx] < 0)
+      continue;
+
+    if (Mask[Idx] < (int)NumSrcElems)
+      TouchesSrc1 = true;
+    else
+      TouchesSrc2 = true;
+  }
+
+  if (!(TouchesSrc1 ^ TouchesSrc2))
+    return false;
+
+  Register NewSrc1 = Shuffle.getSrc1Reg();
+  SmallVector<int, 16> NewMask(Mask);
+  if (TouchesSrc2) {
+    NewSrc1 = Shuffle.getSrc2Reg();
+    commuteMask(NewMask, NumSrcElems);
+  }
+
+  MatchInfo = [=, &Shuffle](MachineIRBuilder &B) {
+    Register Undef = B.buildUndef(Src1Ty).getReg(0);
----------------
aemerson wrote:

> GlobalISel is about weird devices. 

To clarify for people reading this thread in future: no that's not specifically what we designed GlobalISel for. I'm not sure where this notion originated from.

> This rule needs to be more precise. Implicit def must only be legal for register types. That is, for any type that is legal in any operand of any operation, implicit_def must be legal for it.

+1

> I agree that is odd one. I saw something then I am allowed to build it again?

Yes. Whether or not something is legal is context independent. Note there's some subtlety here, you're allowed to make different optimization decisions in the legalizer based on context but the same instruction + type tuple cannot be illegal in one context and legal in another.

If you weren't allowed to assume that an existing tuple in the MIR implied it was ok to build another, then something simple like instruction duplication or even moving would be incorrect. It also prevents us from caching legality which we may want to do in future for compile time performance.

> If you want to build something you must go through isLegalOrBeforeLegalizer .
> COPY might be different. But for G_IMPLCIT_DEF state for which types it is legal (maybe all) in your legalizer. Never ever assume that something is legal

I'm not sure what makes you think that your opinion is the final word on any of this, but this isn't your decision to make unilaterally. Please cool it with this kind of language, I noted on other PRs like Craig Topper's that your wording is unnecessarily harsh. I see it again here.

https://github.com/llvm/llvm-project/pull/115377