[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

76 ↗

(On Diff #101115)

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

239–244 ↗

(On Diff #101115)

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

248 ↗

(On Diff #101115)

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

527–535 ↗

(On Diff #101115)

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.

723–726 ↗

(On Diff #101115)

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 ↗

(On Diff #101126)

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

[rampitec]

lib/Target/AMDGPU/SIFoldOperands.cpp

243–251 ↗

(On Diff #101126)

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

76 ↗

(On Diff #101115)

The whole set is removed.

239–244 ↗

(On Diff #101115)

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

527–535 ↗

(On Diff #101115)

Oops. Leftover from earlier attempts.

[arsenm]

LGTM

lib/Target/AMDGPU/SIFoldOperands.cpp

715 ↗

(On Diff #101126)

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

llvm/trunk/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;
if (!TII->isOperandLegal(*MI, OpNo, OpToFold)) {
     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,
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()) {
+      // Restoring instruction's original operand order if fold has failed.
+      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:

llvm/trunk/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 }

llvm/trunk/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]]

llvm/trunk/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