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

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec added inline comments.


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


================
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);
----------------
arsenm wrote:
> Why not reorder this so the operand check is done before commuting the first time?
isOperandLegal walks actual operands, it may return wrong value if called on an instruction before commuted.


================
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);
+    }
+  };
+
----------------
arsenm wrote:
> 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.
Oops. Leftover from earlier attempts.


================
Comment at: lib/Target/AMDGPU/SIFoldOperands.cpp:243-251
     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);
       return false;
----------------
arsenm wrote:
> This is still undoing the commute, I meant to move the operand legal check before the first commuteInstruction.
It will not work: isOperandLegal walks actual operands, it may return wrong value if called on an instruction before commuted.


Repository:
  rL LLVM

https://reviews.llvm.org/D33802