[AMDGPU] Preserve operand order in SIFoldOperands

[rampitec]

Summary

SIFoldOperands can commute operands even if no folding was done.
This change is to preserve IR is no folding was done.

[arsenm]

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

lib/Target/AMDGPU/SIFoldOperands.cpp

83

This is a set, so it's slightly misleading to call it list. CommutedInsts would be better

243–253

Why not reorder this so the operand check is done before commuting the first time?

252

Why do you need to track the instructions? Can you just add whether the instruction needs commuting to the FoldCandidate?

534–535

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.

715–718

You can just use the simple form of commuteInstruction which calls findCommutedOpIndices for you

[rampitec]

In D33802#770737, @arsenm wrote:

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

It now blindly commutes everything.

[rampitec]

Addressed review comments.

[arsenm]

lib/Target/AMDGPU/SIFoldOperands.cpp

243–251

This is still undoing the commute, I meant to move the operand legal check before the first commuteInstruction.

[rampitec]

lib/Target/AMDGPU/SIFoldOperands.cpp

83

The whole set is removed.

243–251

It will not work: isOperandLegal walks actual operands, it may return wrong value if called on an instruction before commuted.

243–253

isOperandLegal walks actual operands, it may return wrong value if called on an instruction before commuted.

534–535

Oops. Leftover from earlier attempts.

[arsenm]

LGTM

lib/Target/AMDGPU/SIFoldOperands.cpp

715

Should add a comment that this is undoing the commute if the fold failed to restore the instruction to its original order

[rampitec]

Added comment.

[arsenm]

LGTM

lib/Target/AMDGPU/SIFoldOperands.cpp

struct FoldCandidate {
   MachineInstr *UseMI;
   union {
     MachineOperand *OpToFold;
     uint64_t ImmToFold;
     int FrameIndexToFold;
   };
   unsigned char UseOpNo;
   MachineOperand::MachineOperandType Kind;
+  bool Commuted;
 
-  FoldCandidate(MachineInstr *MI, unsigned OpNo, MachineOperand *FoldOp) :
-    UseMI(MI), OpToFold(nullptr), UseOpNo(OpNo), Kind(FoldOp->getType()) {
+  FoldCandidate(MachineInstr *MI, unsigned OpNo, MachineOperand *FoldOp,
+                bool Commuted_ = false) :
+    UseMI(MI), OpToFold(nullptr), UseOpNo(OpNo), Kind(FoldOp->getType()),
+    Commuted(Commuted_) {
     if (FoldOp->isImm()) {
       ImmToFold = FoldOp->getImm();
     } else if (FoldOp->isFI()) {
       FrameIndexToFold = FoldOp->getIndex();
     } else {
       assert(FoldOp->isReg());
       OpToFold = FoldOp;
     }
   }
 
   bool isFI() const {
     return Kind == MachineOperand::MO_FrameIndex;
   }
 
   bool isImm() const {
     return Kind == MachineOperand::MO_Immediate;
   }
 
   bool isReg() const {
     return Kind == MachineOperand::MO_Register;
   }
+
+  bool isCommuted() const {
+    return Commuted;
+  }
 };
 
 class SIFoldOperands : public MachineFunctionPass {
 public:
   static char ID;
   MachineRegisterInfo *MRI;
   const SIInstrInfo *TII;
   const SIRegisterInfo *TRI;
   const SISubtarget *ST;
 
   void foldOperand(MachineOperand &OpToFold,
                    MachineInstr *UseMI,
                    unsigned UseOpIdx,
                    SmallVectorImpl<FoldCandidate> &FoldList,
                    SmallVectorImpl<MachineInstr *> &CopiesToReplace) const;
 
   void foldInstOperand(MachineInstr &MI, MachineOperand &OpToFold) const;
 
   const MachineOperand *isClamp(const MachineInstr &MI) const;
   bool tryFoldClamp(MachineInstr &MI);
 
   std::pair<const MachineOperand *, int> isOMod(const MachineInstr &MI) const;
   bool tryFoldOMod(MachineInstr &MI);
if (!TII->isOperandLegal(*MI, OpNo, OpToFold)) {
     // One of operands might be an Imm operand, and OpNo may refer to it after
     // the call of commuteInstruction() below. Such situations are avoided
     // here explicitly as OpNo must be a register operand to be a candidate
     // for memory folding.
     if (CanCommute && (!MI->getOperand(CommuteIdx0).isReg() ||
                        !MI->getOperand(CommuteIdx1).isReg()))
       return false;
 
     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;
     }
 
+    FoldList.push_back(FoldCandidate(MI, OpNo, OpToFold, true));
+    return true;
+  }
+
   FoldList.push_back(FoldCandidate(MI, OpNo, OpToFold));
   return true;
 }
 
 // If the use operand doesn't care about the value, this may be an operand only
 // used for register indexing, in which case it is unsafe to fold.
 static bool isUseSafeToFold(const SIInstrInfo *TII,
                             const MachineInstr &MI,
static bool tryConstantFoldOp(MachineRegisterInfo &MRI,
   }
 
   if (!MI->isCommutable())
     return false;
 
   if (Src0->isImm() && !Src1->isImm()) {
     std::swap(Src0, Src1);
     std::swap(Src0Idx, Src1Idx);
   }
 
   int32_t Src1Val = static_cast<int32_t>(Src1->getImm());
   if (Opc == AMDGPU::V_OR_B32_e64 ||
       Opc == AMDGPU::V_OR_B32_e32 ||
       Opc == AMDGPU::S_OR_B32) {
     if (Src1Val == 0) {
       // y = or x, 0 => y = copy x
       MI->RemoveOperand(Src1Idx);
       mutateCopyOp(*MI, TII->get(AMDGPU::COPY));
if (updateOperand(Fold, *TRI)) {
         // FIXME: Probably shouldn't bother trying to fold if not an
         // SGPR. PeepholeOptimizer can eliminate redundant VGPR->VGPR
         // copies.
         MRI->clearKillFlags(Fold.OpToFold->getReg());
       }
       DEBUG(dbgs() << "Folded source from " << MI << " into OpNo " <<
             static_cast<int>(Fold.UseOpNo) << " of " << *Fold.UseMI << '\n');
       tryFoldInst(TII, Fold.UseMI);
+    } else if (Fold.isCommuted()) {
+      TII->commuteInstruction(*Fold.UseMI, false);
     }
   }
 }
 
 const MachineOperand *SIFoldOperands::isClamp(const MachineInstr &MI) const {
   unsigned Op = MI.getOpcode();
   switch (Op) {
   case AMDGPU::V_MAX_F32_e64:
   case AMDGPU::V_MAX_F16_e64:
   case AMDGPU::V_MAX_F64: {
     if (!TII->getNamedOperand(MI, AMDGPU::OpName::clamp)->getImm())

test/CodeGen/AMDGPU/commute-compares.ll

define amdgpu_kernel void @commute_ne_64_i32(i32 addrspace(1)* %out, i32 addrspace(1)* %in) #1 {
   %ext = sext i1 %cmp to i32
   store i32 %ext, i32 addrspace(1)* %gep.out
   ret void
 }
 
 ; FIXME: Why isn't this being folded as a constant?
 ; GCN-LABEL: {{^}}commute_ne_litk_i32:
 ; GCN: v_mov_b32_e32 [[K:v[0-9]+]], 0x3039
-; GCN: v_cmp_ne_u32_e32 vcc, [[K]], v{{[0-9]+}}
+; GCN: v_cmp_ne_u32_e32 vcc, v{{[0-9]+}}, [[K]]
 define amdgpu_kernel void @commute_ne_litk_i32(i32 addrspace(1)* %out, i32 addrspace(1)* %in) #1 {
   %tid = call i32 @llvm.amdgcn.workitem.id.x() #0
   %gep.in = getelementptr i32, i32 addrspace(1)* %in, i32 %tid
   %gep.out = getelementptr i32, i32 addrspace(1)* %out, i32 %tid
   %val = load i32, i32 addrspace(1)* %gep.in
   %cmp = icmp ne i32 %val, 12345
   %ext = sext i1 %cmp to i32
   store i32 %ext, i32 addrspace(1)* %gep.out
define amdgpu_kernel void @commute_ule_63_i32(i32 addrspace(1)* %out, i32 addrspace(1)* %in) #1 {
   %gep.out = getelementptr i32, i32 addrspace(1)* %out, i32 %tid
   %val = load i32, i32 addrspace(1)* %gep.in
   %cmp = icmp ule i32 %val, 63
   %ext = sext i1 %cmp to i32
   store i32 %ext, i32 addrspace(1)* %gep.out
   ret void
 }
 
-; FIXME: Undo canonicalization to gt (x + 1) since it doesn't use the inline imm
-
 ; GCN-LABEL: {{^}}commute_ule_64_i32:
 ; GCN: v_mov_b32_e32 [[K:v[0-9]+]], 0x41{{$}}
-; GCN: v_cmp_gt_u32_e32 vcc, [[K]], v{{[0-9]+}}
+; GCN: v_cmp_lt_u32_e32 vcc, v{{[0-9]+}}, [[K]]
 define amdgpu_kernel void @commute_ule_64_i32(i32 addrspace(1)* %out, i32 addrspace(1)* %in) #1 {
   %tid = call i32 @llvm.amdgcn.workitem.id.x() #0
   %gep.in = getelementptr i32, i32 addrspace(1)* %in, i32 %tid
   %gep.out = getelementptr i32, i32 addrspace(1)* %out, i32 %tid
   %val = load i32, i32 addrspace(1)* %gep.in
   %cmp = icmp ule i32 %val, 64
   %ext = sext i1 %cmp to i32
   store i32 %ext, i32 addrspace(1)* %gep.out

 ; FIXME: Should be able to fold this frameindex
 ; Without commuting the frame index in the pre-regalloc run of
 ; SIShrinkInstructions, this was using the VOP3 compare.
 
 ; GCN-LABEL: {{^}}commute_frameindex:
 ; XGCN: v_cmp_eq_u32_e32 vcc, 0, v{{[0-9]+}}
 
 ; GCN: v_mov_b32_e32 [[FI:v[0-9]+]], 4{{$}}
-; GCN: v_cmp_eq_u32_e32 vcc, [[FI]], v{{[0-9]+}}
+; GCN: v_cmp_eq_u32_e32 vcc, v{{[0-9]+}}, [[FI]]
 define amdgpu_kernel void @commute_frameindex(i32 addrspace(1)* nocapture %out) #0 {
 entry:
   %stack0 = alloca i32
   %ptr0 = load volatile i32*, i32* addrspace(1)* undef
   %eq = icmp eq i32* %ptr0, %stack0
   %ext = zext i1 %eq to i32
   store volatile i32 %ext, i32 addrspace(1)* %out
   ret void
 }
 
 attributes #0 = { nounwind readnone }
 attributes #1 = { nounwind }

test/CodeGen/AMDGPU/insert_vector_elt.v2i16.ll

define amdgpu_kernel void @v_insertelement_v2i16_dynamic_sgpr(<2 x i16> addrspace(1)* %out, <2 x i16> addrspace(1)* %in, i32 %idx) #0 {
   %out.gep = getelementptr inbounds <2 x i16>, <2 x i16> addrspace(1)* %out, i64 %tid.ext
   %vec = load <2 x i16>, <2 x i16> addrspace(1)* %in.gep
   %vecins = insertelement <2 x i16> %vec, i16 999, i32 %idx
   store <2 x i16> %vecins, <2 x i16> addrspace(1)* %out.gep
   ret void
 }
 
 ; GCN-LABEL: {{^}}v_insertelement_v2i16_dynamic_vgpr:
-; GFX89: s_mov_b32 [[MASKK:s[0-9]+]], 0xffff{{$}}
-; CI: v_mov_b32_e32 [[K:v[0-9]+]], 0x3e7
 ; GCN: flat_load_dword [[IDX:v[0-9]+]]
 ; GCN: flat_load_dword [[VEC:v[0-9]+]]
-; GFX89-DAG: v_mov_b32_e32 [[K:v[0-9]+]], 0x3e7
+; GFX89-DAG: s_mov_b32 [[MASKK:s[0-9]+]], 0xffff{{$}}
+; GCN-DAG:   v_mov_b32_e32 [[K:v[0-9]+]], 0x3e7
 
 ; GFX89-DAG: v_lshlrev_b32_e32 [[SCALED_IDX:v[0-9]+]], 16, [[IDX]]
 ; GFX89-DAG: v_lshlrev_b32_e64 [[MASK:v[0-9]+]], [[SCALED_IDX]], [[MASKK]]
 
 ; CI-DAG: v_lshlrev_b32_e32 [[SCALED_IDX:v[0-9]+]], 16, [[IDX]]
 ; CI-DAG: v_lshl_b32_e32 [[MASK:v[0-9]+]], 0xffff, [[SCALED_IDX]]
 
 ; GCN: v_bfi_b32 [[RESULT:v[0-9]+]], [[MASK]], [[K]], [[VEC]]
 ; GCN: flat_store_dword v{{\[[0-9]+:[0-9]+\]}}, [[RESULT]]
 define amdgpu_kernel void @v_insertelement_v2i16_dynamic_vgpr(<2 x i16> addrspace(1)* %out, <2 x i16> addrspace(1)* %in, i32 addrspace(1)* %idx.ptr) #0 {
   %tid = call i32 @llvm.amdgcn.workitem.id.x() #1
   %tid.ext = sext i32 %tid to i64
   %in.gep = getelementptr inbounds <2 x i16>, <2 x i16> addrspace(1)* %in, i64 %tid.ext
   %idx.gep = getelementptr inbounds i32, i32 addrspace(1)* %idx.ptr, i64 %tid.ext
   %out.gep = getelementptr inbounds <2 x i16>, <2 x i16> addrspace(1)* %out, i64 %tid.ext
   %idx = load i32, i32 addrspace(1)* %idx.gep
   %vec = load <2 x i16>, <2 x i16> addrspace(1)* %in.gep
   %vecins = insertelement <2 x i16> %vec, i16 999, i32 %idx
   store <2 x i16> %vecins, <2 x i16> addrspace(1)* %out.gep
   ret void
 }
 
 ; GCN-LABEL: {{^}}v_insertelement_v2f16_dynamic_vgpr:
-; GFX89: s_mov_b32 [[MASKK:s[0-9]+]], 0xffff{{$}}
-; CI: v_mov_b32_e32 [[K:v[0-9]+]], 0x1234
 ; GCN: flat_load_dword [[IDX:v[0-9]+]]
 ; GCN: flat_load_dword [[VEC:v[0-9]+]]
-; GFX89-DAG: v_mov_b32_e32 [[K:v[0-9]+]], 0x1234
+; GFX89-DAG: s_mov_b32 [[MASKK:s[0-9]+]], 0xffff{{$}}
+; GCN-DAG:   v_mov_b32_e32 [[K:v[0-9]+]], 0x1234
 
 ; GFX89-DAG: v_lshlrev_b32_e32 [[SCALED_IDX:v[0-9]+]], 16, [[IDX]]
 ; GFX89-DAG: v_lshlrev_b32_e64 [[MASK:v[0-9]+]], [[SCALED_IDX]], [[MASKK]]
 
 ; CI-DAG: v_lshlrev_b32_e32 [[SCALED_IDX:v[0-9]+]], 16, [[IDX]]
 ; CI-DAG: v_lshl_b32_e32 [[MASK:v[0-9]+]], 0xffff, [[SCALED_IDX]]
 
 ; GCN: v_bfi_b32 [[RESULT:v[0-9]+]], [[MASK]], [[K]], [[VEC]]

test/CodeGen/AMDGPU/sub.i16.ll

define amdgpu_kernel void @v_test_sub_i16_zext_to_i32(i32 addrspace(1)* %out, i16 addrspace(1)* %in0, i16 addrspace(1)* %in1) #1 {
   %add = sub i16 %a, %b
   %ext = zext i16 %add to i32
   store i32 %ext, i32 addrspace(1)* %out
   ret void
 }
 
 ; FIXME: Need to handle non-uniform case for function below (load without gep).
 ; GCN-LABEL: {{^}}v_test_sub_i16_zext_to_i64:
+; VI: v_mov_b32_e32 v[[VZERO:[0-9]+]], 0
 ; VI: flat_load_ushort [[A:v[0-9]+]]
 ; VI: flat_load_ushort [[B:v[0-9]+]]
-; VI-DAG: v_mov_b32_e32 v[[VZERO:[0-9]+]], 0
 ; VI-DAG: v_subrev_u16_e32 v[[ADD:[0-9]+]], [[B]], [[A]]
 ; VI: buffer_store_dwordx2 v{{\[}}[[ADD]]:[[VZERO]]{{\]}}, off, {{s\[[0-9]+:[0-9]+\]}}, 0{{$}}
 define amdgpu_kernel void @v_test_sub_i16_zext_to_i64(i64 addrspace(1)* %out, i16 addrspace(1)* %in0, i16 addrspace(1)* %in1) #1 {
   %tid = call i32 @llvm.amdgcn.workitem.id.x()
   %gep.out = getelementptr inbounds i64, i64 addrspace(1)* %out, i32 %tid
   %gep.in0 = getelementptr inbounds i16, i16 addrspace(1)* %in0, i32 %tid
   %gep.in1 = getelementptr inbounds i16, i16 addrspace(1)* %in1, i32 %tid
   %a = load volatile i16, i16 addrspace(1)* %gep.in0