[TII] NFCI: Simplify the interface for isTriviallyReMaterializable

[sdesmalen]

Summary

Currently isTriviallyReMaterializable calls
isReallyTriviallyReMaterializable and
isReallyTriviallyReMaterializableGeneric. The two interfaces
are confusing, but there are also some real issues with this.

The documentation of this function (see below) suggests that
isReallyTriviallyRematerializable allows the target to override the
default behaviour.

/// 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.

It however implements something different. The default behaviour
is the analysis done in isReallyTriviallyReMaterializableGeneric,
which is testing if it is safe to rematerialize the MachineInstr.

The result of isReallyTriviallyReMaterializable is only considered if
isReallyTriviallyReMaterializableGeneric returns false. That means
there is no way to override the default behaviour if
isReallyTriviallyReMaterializableGeneric returns true (i.e. it is safe to
rematerialize, but we'd rather not).

By making this a single interface, we can override the interface to do either.

[arsenm]

This interface never made any sense

[sdesmalen]

In D156520#4541898, @arsenm wrote:

This interface never made any sense

Agreed. I still find the need for isReallyTriviallyReMaterializable() a bit confusing, over simply having only isTriviallyReMaterializable(), but I didn't want to complicate this patch too much. It is possible to remove this layer, but that does complicate the overriden Target methods a bit more.

[piotr]

Thanks, AMD changes LGTM. I spotted this weirdness recently when staring at some remat code for AMDGPU. (It's not a serious issue for us, because the generic check is really conservative for our target, and we typically do not want to block the remat when the generic check returns true).

llvm/include/llvm/MC/MCInstrDesc.h

522–523

"method is" or "methods are"

llvm/lib/Target/X86/X86InstrInfo.cpp

877

Doesn't it need the conditional change as in line 898 below?

[sdesmalen]

Addressed comments

llvm/lib/Target/X86/X86InstrInfo.cpp

877

Good spot, thank you!

[sdesmalen]

Gentle ping

[craig.topper]

LGTM

[nemanjai]

Seems fine to me - certainly from a PPC perspective.

llvm/include/llvm/CodeGen/MachineInstr.h

public:
   /// the conditional flag register is used.
   bool hasPostISelHook(QueryType Type = IgnoreBundle) const {
     return hasProperty(MCID::HasPostISelHook, Type);
   }
 
   /// Returns true if this instruction is a candidate for remat.
   /// This flag is deprecated, please don't use it anymore.  If this
   /// flag is set, the isReallyTriviallyReMaterializable() method is called to
-  /// verify the instruction is really rematable.
+  /// verify the instruction is really rematerializable.
   bool isRematerializable(QueryType Type = AllInBundle) const {
     // It's only possible to re-mat a bundle if all bundled instructions are
     // re-materializable.
     return hasProperty(MCID::Rematerializable, Type);
   }
 
   /// Returns true if this instruction has the same cost (or less) than a move
   /// instruction. This is useful during certain types of optimizations

llvm/include/llvm/CodeGen/TargetInstrInfo.h

public:
   /// Return true if the instruction is trivially rematerializable, meaning it
   /// has no side effects and requires no operands that aren't always available.
   /// This means the only allowed uses are constants and unallocatable physical
   /// registers so that the instructions result is independent of the place
   /// in the function.
   bool isTriviallyReMaterializable(const MachineInstr &MI) const {
     return MI.getOpcode() == TargetOpcode::IMPLICIT_DEF ||
            (MI.getDesc().isRematerializable() &&
-            (isReallyTriviallyReMaterializable(MI) ||
-             isReallyTriviallyReMaterializableGeneric(MI)));
+            isReallyTriviallyReMaterializable(MI));
   }
 
   /// Given \p MO is a PhysReg use return if it can be ignored for the purpose
   /// of instruction rematerialization or sinking.
   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() .
-  virtual bool isReallyTriviallyReMaterializable(const MachineInstr &MI) const {
-    return false;
-  }
+  virtual bool isReallyTriviallyReMaterializable(const MachineInstr &MI) const;
 
   /// This method commutes the operands of the given machine instruction MI.
   /// The operands to be commuted are specified by their indices OpIdx1 and
   /// OpIdx2.
   ///
   /// If a target has any instructions that are commutable but require
   /// converting to different instructions or making non-trivial changes
   /// to commute them, this method can be overloaded to do that.
protected:
   /// The function returns true iff after the result pair redefinition
   /// the fixed result pair is equal to or equivalent to the source pair of
   /// indices: (CommutableOpIdx1, CommutableOpIdx2). It is assumed here that
   /// the pairs (x,y) and (y,x) are equivalent.
   static bool fixCommutedOpIndices(unsigned &ResultIdx1, unsigned &ResultIdx2,
                                    unsigned CommutableOpIdx1,
                                    unsigned CommutableOpIdx2);
 
-private:
-  /// For instructions with opcodes for which the M_REMATERIALIZABLE flag is
-  /// set and the target hook isReallyTriviallyReMaterializable returns false,
-  /// this function does target-independent tests to determine if the
-  /// instruction is really trivially rematerializable.
-  bool isReallyTriviallyReMaterializableGeneric(const MachineInstr &MI) const;
-
 public:
   /// These methods return the opcode of the frame setup/destroy instructions
   /// if they exist (-1 otherwise).  Some targets use pseudo instructions in
   /// order to abstract away the difference between operating with a frame
   /// pointer and operating without, through the use of these two instructions.
   ///
   unsigned getCallFrameSetupOpcode() const { return CallFrameSetupOpcode; }
   unsigned getCallFrameDestroyOpcode() const { return CallFrameDestroyOpcode; }

llvm/include/llvm/MC/MCInstrDesc.h

public:
   /// instruction selection by calling a target hook. For example, this can be
   /// used to fill in ARM 's' optional operand depending on whether the
   /// conditional flag register is used.
   bool hasPostISelHook() const { return Flags & (1ULL << MCID::HasPostISelHook); }
 
   /// Returns true if this instruction is a candidate for remat. This
   /// flag is only used in TargetInstrInfo method isTriviallyRematerializable.
   ///
-  /// If this flag is set, the isReallyTriviallyReMaterializable()
-  /// or isReallyTriviallyReMaterializableGeneric methods are called to verify
-  /// the instruction is really rematable.
+  /// If this flag is set, the isReallyTriviallyReMaterializable() method is
+  /// called to verify the instruction is really rematerializable.
   bool isRematerializable() const {
     return Flags & (1ULL << MCID::Rematerializable);
   }
 
   /// Returns true if this instruction has the same cost (or less) than a
   /// move instruction. This is useful during certain types of optimizations
   /// (e.g., remat during two-address conversion or machine licm) where we would
   /// like to remat or hoist the instruction, but not if it costs more than

llvm/lib/CodeGen/TargetInstrInfo.cpp

void TargetInstrInfo::genAlternativeCodeSequence(
 
   reassociateOps(Root, *Prev, Pattern, InsInstrs, DelInstrs, InstIdxForVirtReg);
 }
 
 MachineTraceStrategy TargetInstrInfo::getMachineCombinerTraceStrategy() const {
   return MachineTraceStrategy::TS_MinInstrCount;
 }
 
-bool TargetInstrInfo::isReallyTriviallyReMaterializableGeneric(
+bool TargetInstrInfo::isReallyTriviallyReMaterializable(
     const MachineInstr &MI) const {
   const MachineFunction &MF = *MI.getMF();
   const MachineRegisterInfo &MRI = MF.getRegInfo();
 
   // Remat clients assume operand 0 is the defined register.
   if (!MI.getNumOperands() || !MI.getOperand(0).isReg())
     return false;
   Register DefReg = MI.getOperand(0).getReg();

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

if (isVOP1(MI) || isVOP2(MI) || isVOP3(MI) || isSDWA(MI) || isSALU(MI)) {
 
     // Another potential implicit use is mode register. The core logic of
     // the RA will not attempt rematerialization if mode is set anywhere
     // in the function, otherwise it is safe since mode is not changed.
 
     // There is difference to generic method which does not allow
     // rematerialization if there are virtual register uses. We allow this,
     // therefore this method includes SOP instructions as well.
-    return !MI.hasImplicitDef() &&
-           MI.getNumImplicitOperands() == MI.getDesc().implicit_uses().size() &&
-           !MI.mayRaiseFPException();
+    if (!MI.hasImplicitDef() &&
+        !MI.mayRaiseFPException())
+      return true;
   }
 
-  return false;
+  return TargetInstrInfo::isReallyTriviallyReMaterializable(MI);
 }
 
 // Returns true if the scalar result of a VALU instruction depends on exec.
 static bool resultDependsOnExec(const MachineInstr &MI) {
   // Ignore comparisons which are only used masked with exec.
   // This allows some hoisting/sinking of VALU comparisons.
   if (MI.isCompare()) {
     const MachineRegisterInfo &MRI = MI.getParent()->getParent()->getRegInfo();

llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp

 }
 
 bool ARMBaseInstrInfo::isReallyTriviallyReMaterializable(
     const MachineInstr &MI) const {
   // Try hard to rematerialize any VCTPs because if we spill P0, it will block
   // the tail predication conversion. This means that the element count
   // register has to be live for longer, but that has to be better than
   // spill/restore and VPT predication.
-  return isVCTP(&MI) && !isPredicated(MI);
+  return (isVCTP(&MI) && !isPredicated(MI)) ||
+         TargetInstrInfo::isReallyTriviallyReMaterializable(MI);
 }
 
 unsigned llvm::getBLXOpcode(const MachineFunction &MF) {
   return (MF.getSubtarget<ARMSubtarget>().hardenSlsBlr()) ? ARM::BLX_noip
                                                           : ARM::BLX;
 }
 
 unsigned llvm::gettBLXrOpcode(const MachineFunction &MF) {

llvm/lib/Target/PowerPC/PPCInstrInfo.cpp

bool PPCInstrInfo::isReallyTriviallyReMaterializable(
   case PPC::V_SETALLONESH:
   case PPC::V_SETALLONES:
   case PPC::CRSET:
   case PPC::CRUNSET:
   case PPC::XXSETACCZ:
   case PPC::XXSETACCZW:
     return true;
   }
-  return false;
+  return TargetInstrInfo::isReallyTriviallyReMaterializable(MI);
 }
 
 unsigned PPCInstrInfo::isStoreToStackSlot(const MachineInstr &MI,
                                           int &FrameIndex) const {
   if (llvm::is_contained(getStoreOpcodesForSpillArray(), MI.getOpcode())) {
     if (MI.getOperand(1).isImm() && !MI.getOperand(1).getImm() &&
         MI.getOperand(2).isFI()) {
       FrameIndex = MI.getOperand(2).getIndex();

llvm/lib/Target/WebAssembly/WebAssemblyInstrInfo.cpp

 
 bool WebAssemblyInstrInfo::isReallyTriviallyReMaterializable(
     const MachineInstr &MI) const {
   switch (MI.getOpcode()) {
   case WebAssembly::CONST_I32:
   case WebAssembly::CONST_I64:
   case WebAssembly::CONST_F32:
   case WebAssembly::CONST_F64:
-    // isReallyTriviallyReMaterializableGeneric misses these because of the
-    // ARGUMENTS implicit def, so we manualy override it here.
+    // TargetInstrInfo::isReallyTriviallyReMaterializable misses these
+    // because of the ARGUMENTS implicit def, so we manualy override it here.
     return true;
   default:
-    return false;
+    return TargetInstrInfo::isReallyTriviallyReMaterializable(MI);
   }
 }
 
 void WebAssemblyInstrInfo::copyPhysReg(MachineBasicBlock &MBB,
                                        MachineBasicBlock::iterator I,
                                        const DebugLoc &DL, MCRegister DestReg,
                                        MCRegister SrcReg, bool KillSrc) const {
   // This method is called by post-RA expansion, which expects only pregs to

llvm/lib/Target/X86/X86InstrInfo.cpp

if (MI.getOperand(1 + X86::AddrBaseReg).isReg() &&
         MI.getOperand(1 + X86::AddrScaleAmt).isImm() &&
         MI.getOperand(1 + X86::AddrIndexReg).isReg() &&
         MI.getOperand(1 + X86::AddrIndexReg).getReg() == 0 &&
         MI.isDereferenceableInvariantLoad()) {
       Register BaseReg = MI.getOperand(1 + X86::AddrBaseReg).getReg();
       if (BaseReg == 0 || BaseReg == X86::RIP)
         return true;
       // Allow re-materialization of PIC load.
-      if (!ReMatPICStubLoad && MI.getOperand(1 + X86::AddrDisp).isGlobal())
-        return false;
+      if (!(!ReMatPICStubLoad && MI.getOperand(1 + X86::AddrDisp).isGlobal())) {
         const MachineFunction &MF = *MI.getParent()->getParent();
         const MachineRegisterInfo &MRI = MF.getRegInfo();
-      return regIsPICBase(BaseReg, MRI);
+        if (regIsPICBase(BaseReg, MRI))
+          return true;
       }
-    return false;
+    }
+    break;
   }
 
   case X86::LEA32r:
   case X86::LEA64r: {
     if (MI.getOperand(1 + X86::AddrScaleAmt).isImm() &&
         MI.getOperand(1 + X86::AddrIndexReg).isReg() &&
         MI.getOperand(1 + X86::AddrIndexReg).getReg() == 0 &&
         !MI.getOperand(1 + X86::AddrDisp).isReg()) {
       // lea fi#, lea GV, etc. are all rematerializable.
       if (!MI.getOperand(1 + X86::AddrBaseReg).isReg())
         return true;
       Register BaseReg = MI.getOperand(1 + X86::AddrBaseReg).getReg();
       if (BaseReg == 0)
         return true;
       // Allow re-materialization of lea PICBase + x.
       const MachineFunction &MF = *MI.getParent()->getParent();
       const MachineRegisterInfo &MRI = MF.getRegInfo();
-      return regIsPICBase(BaseReg, MRI);
+      if (regIsPICBase(BaseReg, MRI))
+        return true;
     }
-    return false;
+    break;
   }
   }
+  return TargetInstrInfo::isReallyTriviallyReMaterializable(MI);
 }
 
 void X86InstrInfo::reMaterialize(MachineBasicBlock &MBB,
                                  MachineBasicBlock::iterator I,
                                  Register DestReg, unsigned SubIdx,
                                  const MachineInstr &Orig,
                                  const TargetRegisterInfo &TRI) const {
   bool ClobbersEFLAGS = Orig.modifiesRegister(X86::EFLAGS, &TRI);