[AMDGPU] Add TII::isIgnorableUse() to allow VOP rematerialization

[rampitec]

Summary

Any def of EXEC prevents rematerialization of any VOP instruction
because of the physreg use. Create a callback to check if the
physreg use can be ingored to allow rematerialization.

[arsenm]

Commit message should say TII

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

127

This also needs to verify it's a VALU use, you could still have an implicit exec read of a scalar instruction

[rampitec]

Check instruction is VALU.

[rampitec]

PSDB and ePSDB passed.

[foad]

Typo in summary "rematerialization".

llvm/lib/CodeGen/LiveRangeEdit.cpp

116–117

Update this comment?

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

126

Typo "And".

[rampitec]

Updated comments.

[kparzysz]

The definition of "ambient" is so vague that I have no idea what it's trying to convey.

There is already a concept of "pristine" registers, which sounds similar. How is this different?

[rampitec]

In D105836#2877339, @kparzysz wrote:

The definition of "ambient" is so vague that I have no idea what it's trying to convey.

There is already a concept of "pristine" registers, which sounds similar. How is this different?

It is not the same as pristine registers. For pristine registers the idea is that value in a register must be preserved but actually unused in a function. In this case value is not preserved and actually may be used, it is just not directly used in an instruction. The EXEC register holds "execution mask", essentially a mask of active threads. We add it as an implicit operand to all VALU instructions to model the dependency on this mask. However, these instructions do not really read EXEC, it is just a predicate. This implicit use is more or less an "environment" thing.

I agree that "ambient" sounds vague and would appreciate any suggestion.

[kparzysz]

I think it would be a lot better to add the concept of a "rematerializable" physical register. I think that currently physical registers are considered volatile, unless they are constant, but there was some talk a while back about refining the classification to allow exactly this sort of thing.

[rampitec]

In D105836#2877641, @kparzysz wrote:

I think it would be a lot better to add the concept of a "rematerializable" physical register. I think that currently physical registers are considered volatile, unless they are constant, but there was some talk a while back about refining the classification to allow exactly this sort of thing.

There was such idea, but it unfortunately not applicable here. SALU instructions and even some VALU instructions can use EXEC as a data operand. That is only a special implicit use on a VALU instruction is a non-data use to be ignored.

[rampitec]

Maybe rename it to isRematerializableUse()? At least it will be a specific name.

[kparzysz]

Yes, that's what I meant. I.e. not "it this register always rematerializable", but "it this register rematerializable in this situation".

It will require more work, because we may need to insert instructions that restore the value of that physical register, and currently that's not happening.

[arsenm]

In D105836#2877763, @kparzysz wrote:

Yes, that's what I meant. I.e. not "it this register always rematerializable", but "it this register rematerializable in this situation".

It will require more work, because we may need to insert instructions that restore the value of that physical register, and currently that's not happening.

We specifically do not want anything trying to manage the value of the register here. It's a predicate register and we want the value to be whatever it happens to be at that point. If the predicate was on for a lane at the def, it's fine if that bit is off at the use. The inverse would be an invalid situation to begin with

[kparzysz]

In D105836#2877775, @arsenm wrote:

We specifically do not want anything trying to manage the value of the register here. It's a predicate register and we want the value to be whatever it happens to be at that point. If the predicate was on for a lane at the def, it's fine if that bit is off at the use. The inverse would be an invalid situation to begin with

So you don't care if exec has a different value at OldIdx and UseIdx?

[arsenm]

In D105836#2877797, @kparzysz wrote:

In D105836#2877775, @arsenm wrote:

We specifically do not want anything trying to manage the value of the register here. It's a predicate register and we want the value to be whatever it happens to be at that point. If the predicate was on for a lane at the def, it's fine if that bit is off at the use. The inverse would be an invalid situation to begin with

So you don't care if exec has a different value at OldIdx and UseIdx?

Yes. It's a bitmask of active lanes. It's safe to rematerialize the instruction for lanes which are active at the def, but only a subset are at the use which would look like a different register value

[kparzysz]

Then either implement "isRematerializableUse", and add support for rematerializing it that does nothing for your target, or change it to something like "isIgnorableUse".

What I'm trying to accomplish is to make it applicable to other targets as well, so I'm trying to classify it as a special case of an issue that can occur elsewhere.

[rampitec]

Renamed callback to isIgnorableUse().

llvm/include/llvm/CodeGen/TargetInstrInfo.h

public:
   bool isTriviallyReMaterializable(const MachineInstr &MI,
                                    AAResults *AA = nullptr) const {
     return MI.getOpcode() == TargetOpcode::IMPLICIT_DEF ||
            (MI.getDesc().isRematerializable() &&
             (isReallyTriviallyReMaterializable(MI, AA) ||
              isReallyTriviallyReMaterializableGeneric(MI, AA)));
   }
 
+  /// Given \p MO is a PhysReg use return if it can be ignored for the purpose
+  /// of instruction rematerialization.
+  virtual bool isIgnorableUse(const MachineOperand &MO) const {
+    return false;
+  }
+
 protected:
   /// For instructions with opcodes for which the M_REMATERIALIZABLE flag is
   /// set, this hook lets the target specify whether the instruction is actually
   /// trivially rematerializable, taking into consideration its operands. This
   /// predicate must return false if the instruction has any side effects other
   /// than producing a value, or if it requres any address registers that are
   /// not always available.
   /// Requirements must be check as stated in isTriviallyReMaterializable() .

llvm/lib/CodeGen/LiveRangeEdit.cpp

bool LiveRangeEdit::allUsesAvailableAt(const MachineInstr *OrigMI,
                                        SlotIndex UseIdx) const {
   OrigIdx = OrigIdx.getRegSlot(true);
   UseIdx = UseIdx.getRegSlot(true);
   for (unsigned i = 0, e = OrigMI->getNumOperands(); i != e; ++i) {
     const MachineOperand &MO = OrigMI->getOperand(i);
     if (!MO.isReg() || !MO.getReg() || !MO.readsReg())
       continue;
 
-    // We can't remat physreg uses, unless it is a constant.
+    // We can't remat physreg uses, unless it is a constant or target wants
+    // to ignore this use.
     if (Register::isPhysicalRegister(MO.getReg())) {
-      if (MRI.isConstantPhysReg(MO.getReg()))
+      if (MRI.isConstantPhysReg(MO.getReg()) || TII.isIgnorableUse(MO))
         continue;
       return false;
     }
 
     LiveInterval &li = LIS.getInterval(MO.getReg());
     const VNInfo *OVNI = li.getVNInfoAt(OrigIdx);
     if (!OVNI)
       continue;

llvm/lib/Target/AMDGPU/SIInstrInfo.h

public:
 
   const GCNSubtarget &getSubtarget() const {
     return ST;
   }
 
   bool isReallyTriviallyReMaterializable(const MachineInstr &MI,
                                          AAResults *AA) const override;
 
+  bool isIgnorableUse(const MachineOperand &MO) const override;
+
   bool areLoadsFromSameBasePtr(SDNode *Load1, SDNode *Load2,
                                int64_t &Offset1,
                                int64_t &Offset2) const override;
 
   bool getMemOperandsWithOffsetWidth(
       const MachineInstr &LdSt,
       SmallVectorImpl<const MachineOperand *> &BaseOps, int64_t &Offset,
       bool &OffsetIsScalable, unsigned &Width,

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

if (isVOP1(MI) || isVOP3(MI) || isSDWA(MI)) {
     // in the function, otherwise it is safe since mode is not changed.
     return !MI.hasImplicitDef() &&
            MI.getNumImplicitOperands() == MI.getDesc().getNumImplicitUses();
   }
 
   return false;
 }
 
+bool SIInstrInfo::isIgnorableUse(const MachineOperand &MO) const {
+  // Any implicit use of exec by VALU is not a real register read.
+  return MO.getReg() == AMDGPU::EXEC && MO.isImplicit() &&
+         isVALU(*MO.getParent());
+}
+
 bool SIInstrInfo::areLoadsFromSameBasePtr(SDNode *Load0, SDNode *Load1,
                                           int64_t &Offset0,
                                           int64_t &Offset1) const {
   if (!Load0->isMachineOpcode() || !Load1->isMachineOpcode())
     return false;
 
   unsigned Opc0 = Load0->getMachineOpcode();
   unsigned Opc1 = Load1->getMachineOpcode();

llvm/test/CodeGen/AMDGPU/remat-sop.mir

bb.0:
     %1:sreg_32 = S_MOV_B32 2
     %2:sreg_32 = S_MOV_B32 3
     S_NOP 0, implicit %0
     S_NOP 0, implicit %1
     S_NOP 0, implicit %2
     S_ENDPGM 0
 ...
 ---
+name:            test_no_remat_s_mov_b32_impuse_exec
+tracksRegLiveness: true
+machineFunctionInfo:
+  stackPtrOffsetReg:  $sgpr32
+body:             |
+  bb.0:
+    ; GCN-LABEL: name: test_no_remat_s_mov_b32_impuse_exec
+    ; GCN: $exec = IMPLICIT_DEF
+    ; GCN: renamable $sgpr0 = S_MOV_B32 1, implicit $exec
+    ; GCN: SI_SPILL_S32_SAVE killed renamable $sgpr0, %stack.1, implicit $exec, implicit $sgpr32 :: (store (s32) into %stack.1, addrspace 5)
+    ; GCN: renamable $sgpr1 = S_MOV_B32 2, implicit $exec
+    ; GCN: renamable $sgpr0 = S_MOV_B32 3, implicit $exec
+    ; GCN: SI_SPILL_S32_SAVE killed renamable $sgpr0, %stack.0, implicit $exec, implicit $sgpr32 :: (store (s32) into %stack.0, addrspace 5)
+    ; GCN: renamable $sgpr0 = SI_SPILL_S32_RESTORE %stack.1, implicit $exec, implicit $sgpr32 :: (load (s32) from %stack.1, addrspace 5)
+    ; GCN: S_NOP 0, implicit killed renamable $sgpr0
+    ; GCN: S_NOP 0, implicit killed renamable $sgpr1
+    ; GCN: renamable $sgpr0 = SI_SPILL_S32_RESTORE %stack.0, implicit $exec, implicit $sgpr32 :: (load (s32) from %stack.0, addrspace 5)
+    ; GCN: S_NOP 0, implicit killed renamable $sgpr0
+    ; GCN: S_ENDPGM 0
+    $exec = IMPLICIT_DEF
+    %0:sreg_32 = S_MOV_B32 1, implicit $exec
+    %1:sreg_32 = S_MOV_B32 2, implicit $exec
+    %2:sreg_32 = S_MOV_B32 3, implicit $exec
+    S_NOP 0, implicit %0
+    S_NOP 0, implicit %1
+    S_NOP 0, implicit %2
+    S_ENDPGM 0
+...
+---
 name:            test_remat_s_mov_b64
 tracksRegLiveness: true
 body:             |
   bb.0:
     ; GCN-LABEL: name: test_remat_s_mov_b64
     ; GCN: renamable $sgpr0_sgpr1 = S_MOV_B64 1
     ; GCN: renamable $sgpr2_sgpr3 = S_MOV_B64 2
     ; GCN: S_NOP 0, implicit killed renamable $sgpr0_sgpr1

llvm/test/CodeGen/AMDGPU/remat-vop.mir

bb.0:
     %1:vgpr_32 = V_MOV_B32_e32 2, implicit $exec, implicit $m0
     %2:vgpr_32 = V_MOV_B32_e32 3, implicit $exec, implicit $m0
     S_NOP 0, implicit %0
     S_NOP 0, implicit %1
     S_NOP 0, implicit %2
     S_ENDPGM 0
 ...
 ---
+name:            test_remat_v_mov_b32_e32_exec_def
+tracksRegLiveness: true
+machineFunctionInfo:
+  stackPtrOffsetReg:  $sgpr32
+body:             |
+  bb.0:
+    ; GCN-LABEL: name: test_remat_v_mov_b32_e32_exec_def
+    ; GCN: renamable $vgpr0 = V_MOV_B32_e32 1, implicit $exec
+    ; GCN: renamable $vgpr1 = V_MOV_B32_e32 2, implicit $exec
+    ; GCN: S_NOP 0, implicit killed renamable $vgpr0
+    ; GCN: S_NOP 0, implicit killed renamable $vgpr1
+    ; GCN: renamable $vgpr0 = V_MOV_B32_e32 3, implicit $exec
+    ; GCN: S_NOP 0, implicit killed renamable $vgpr0
+    ; GCN: $exec = S_ANDN2_B64_term $exec, undef renamable $sgpr0_sgpr1, implicit-def $scc
+    ; GCN: S_ENDPGM 0
+    %0:vgpr_32 = V_MOV_B32_e32 1, implicit $exec
+    %1:vgpr_32 = V_MOV_B32_e32 2, implicit $exec
+    %2:vgpr_32 = V_MOV_B32_e32 3, implicit $exec
+    S_NOP 0, implicit %0
+    S_NOP 0, implicit %1
+    S_NOP 0, implicit %2
+    $exec = S_ANDN2_B64_term $exec, undef %4:sreg_64, implicit-def $scc
+    S_ENDPGM 0
+...
+---
 name:            test_remat_v_mov_b32_e64
 tracksRegLiveness: true
 body:             |
   bb.0:
     ; GCN-LABEL: name: test_remat_v_mov_b32_e64
     ; GCN: renamable $vgpr0 = V_MOV_B32_e64 1, implicit $exec
     ; GCN: renamable $vgpr1 = V_MOV_B32_e64 2, implicit $exec
     ; GCN: S_NOP 0, implicit killed renamable $vgpr0