[PATCH] D33802: [AMDGPU] Preserve operand order in SIFoldOperands

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added a comment.

It would make sense to compute even if nothing is folded, since having constants in src0 as the canonical order would make sense, although I think it blindly commutes still as it is



================
Comment at: lib/Target/AMDGPU/SIFoldOperands.cpp:76
                    SmallVectorImpl<FoldCandidate> &FoldList,
+                   SmallSet<MachineInstr *, 4> &CommutedList,
                    SmallVectorImpl<MachineInstr *> &CopiesToReplace) const;
----------------
This is a set, so it's slightly misleading to call it list. CommutedInsts would be better


================
Comment at: lib/Target/AMDGPU/SIFoldOperands.cpp:239-244
     if (!CanCommute ||
         !TII->commuteInstruction(*MI, false, CommuteIdx0, CommuteIdx1))
       return false;
 
-    if (!TII->isOperandLegal(*MI, OpNo, OpToFold))
+    if (!TII->isOperandLegal(*MI, OpNo, OpToFold)) {
+      TII->commuteInstruction(*MI, false, CommuteIdx0, CommuteIdx1);
----------------
Why not reorder this so the operand check is done before commuting the first time?


================
Comment at: lib/Target/AMDGPU/SIFoldOperands.cpp:248
+
+    CommutedList.insert(MI);
   }
----------------
Why do you need to track the instructions? Can you just add whether the instruction needs commuting to the FoldCandidate?


================
Comment at: lib/Target/AMDGPU/SIFoldOperands.cpp:527-535
+  auto trySwapOperands = [&Src0, &Src1, &Src0Idx, &Src1Idx] {
+    if (Src0->isImm() && !Src1->isImm()) {
+      std::swap(Src0, Src1);
+      std::swap(Src0Idx, Src1Idx);
+    }
+  };
+
----------------
Why is changing the constant folding necessary? Nothing is actually commuted here, the instruction isn't mutated. This only does anything if the instruction is being replaced with another instruction anyway.


================
Comment at: lib/Target/AMDGPU/SIFoldOperands.cpp:723-726
+      unsigned CommuteIdx0 = TargetInstrInfo::CommuteAnyOperandIndex;
+      unsigned CommuteIdx1 = TargetInstrInfo::CommuteAnyOperandIndex;
+      TII->findCommutedOpIndices(*Fold.UseMI, CommuteIdx0, CommuteIdx1);
+      TII->commuteInstruction(*Fold.UseMI, false, CommuteIdx0, CommuteIdx1);
----------------
You can just use the simple form of commuteInstruction which calls findCommutedOpIndices for you


Repository:
  rL LLVM

https://reviews.llvm.org/D33802