[llvm] [AMDGPU] Account for existing SDWA selections (PR #123221)

[Frederik Harwath via llvm-commits]

================
@@ -108,6 +111,42 @@ class SDWAOperand {
   MachineOperand *getReplacedOperand() const { return Replaced; }
   MachineInstr *getParentInst() const { return Target->getParent(); }
 
+  /// Combine an SDWA instruction's existing SDWA selection \p
+  /// ExistingSel with the SDWA selection \p OpSel of its operand. If
+  /// the selections are compatible, return the combined selection,
+  /// otherwise return a nullopt. For example, if we have ExistingSel
+  /// = BYTE_0 Sel and FoldedSel WORD_1 Sel:
+  ///     BYTE_0 Sel (WORD_1 Sel (%X)) -> BYTE_2 Sel (%X)
+  std::optional<SdwaSel> combineSdwaSel(SdwaSel ExistingSel,
----------------
frederik-h wrote:

We could split of an `areSelectionsCompatible` predicate from this function and use it to check whether or not this function will be successful before we call it instead of using the `std::optional`. But considering the way in which `SDWASrcOperand::convertToSDWA` determines `SrcSel`, I don't see how we could move this check much above the current place at which we invoke `combineSdwaSel` in this function. Perhaps placing it above the `V_MAC` handling would work. And I am not sure if splitting the function would really improve readability. Here's how the function would roughly look like (still using the `std::optional` which I had not yet removed): 
```
diff --git a/llvm/lib/Target/AMDGPU/SIPeepholeSDWA.cpp b/llvm/lib/Target/AMDGPU/SIPeepholeSDWA.cpp
index 39fe65e7c375..d87072ea88eb 100644
--- a/llvm/lib/Target/AMDGPU/SIPeepholeSDWA.cpp
+++ b/llvm/lib/Target/AMDGPU/SIPeepholeSDWA.cpp
@@ -111,31 +111,33 @@ public:
   MachineOperand *getReplacedOperand() const { return Replaced; }
   MachineInstr *getParentInst() const { return Target->getParent(); }

+  bool compatibleSelections(SdwaSel ExistingSel, SdwaSel OperandSel) {
+    return ExistingSel == SdwaSel::DWORD || OperandSel == ExistingSel ||
+           (ExistingSel != SdwaSel::WORD_1 && ExistingSel != SdwaSel::BYTE_2 &&
+            ExistingSel != SdwaSel::BYTE_3 &&
+            (OperandSel == SdwaSel::WORD_0 || OperandSel == SdwaSel::WORD_1));
+  }
+
   /// Combine an SDWA instruction's existing SDWA selection \p
   /// ExistingSel with the SDWA selection \p OpSel of its operand. If
   /// the selections are compatible, return the combined selection,
   /// otherwise return a nullopt. For example, if we have ExistingSel
-  /// = BYTE_0 Sel and FoldedSel WORD_1 Sel:
+  /// = BYTE_0 Sel and OperandSel WORD_1 Sel:
   ///     BYTE_0 Sel (WORD_1 Sel (%X)) -> BYTE_2 Sel (%X)
   std::optional<SdwaSel> combineSdwaSel(SdwaSel ExistingSel,
                                         SdwaSel OperandSel) {
-    if (ExistingSel == SdwaSel::DWORD)
-      return OperandSel;
-
-    if (OperandSel == SdwaSel::DWORD)
-      return ExistingSel;
-
-    if (ExistingSel == SdwaSel::WORD_1 || ExistingSel == SdwaSel::BYTE_2 ||
-        ExistingSel == SdwaSel::BYTE_3)
+    if (!compatibleSelections(ExistingSel, OperandSel))
       return {};

-    if (ExistingSel == OperandSel)
-      return ExistingSel;
+    if (ExistingSel == SdwaSel::DWORD)
+      return OperandSel;

-    if (OperandSel == SdwaSel::WORD_0)
+    if (OperandSel == ExistingSel ||
+       OperandSel == SdwaSel::DWORD ||
+       OperandSel == SdwaSel::WORD_0)
       return ExistingSel;

-    if (OperandSel == SdwaSel::WORD_1) {
+        if (OperandSel == SdwaSel::WORD_1) {
       if (ExistingSel == SdwaSel::BYTE_0)
         return SdwaSel::BYTE_2;
       if (ExistingSel == SdwaSel::BYTE_1)
@@ -143,8 +145,7 @@ public:
       if (ExistingSel == SdwaSel::WORD_0)
         return SdwaSel::WORD_1;
     }
-
-    return {};
+    llvm_unreachable("Unreachable");
   }

   MachineRegisterInfo *getMRI() const {
``` 

Do you think this is preferable to the current approach?

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