[MIR][M68K] (Patch 2/8): Changes on Target-independent MIR part

[myhsu]

Summary

1. Use the new interface TargetInstrInfo::isRegisterOperandPCRel to tell MachineVerifier whether a specific register operand is legal to have OPERAND_PCREL. Since some of the pc-relative addressing modes in M68K involve (non-pc) registers.
2. Make MachineBasicBlock::findDebugLoc to work on reverse_iterator.

[jrtc27]

llvm/lib/CodeGen/MachineVerifier.cpp

1687–1688

I wonder whether this should be a TII (or similar) hook/field instead; weakening assertions for targets where the assertion is valid is a bit sad.

[myhsu]

Fix formatting issues

[myhsu]

Add a new interface in TargetInstrInfo: isRegisterOperandPCRel to tell MachineVerifier whether specific register operand is legal to be pc-relative

[craig.topper]

llvm/lib/CodeGen/MachineBasicBlock.cpp

1387

Is it possible MBBI is already at instr_rend() so that std::prev isn't valid?

[rengolin]

llvm/include/llvm/CodeGen/TargetInstrInfo.h

956

Shouldn't you also have added the derived implementations for each target?

[myhsu]

llvm/include/llvm/CodeGen/TargetInstrInfo.h

956

I'm not sure, is there a policy to enforce this? Because there is a default implementation here (i.e. return false) so functionally it works. Also some of the functions, the isCopyInstrImpl below for example, don't have implementations in every derived targets either

[rengolin]

llvm/include/llvm/CodeGen/TargetInstrInfo.h

956

There is no policy regarding this. Just now reading the MachineVerrifier again, it seems as this does not change the implementation for any other target that doesn't implement. No worries.

[RKSimon]

llvm/lib/CodeGen/MachineBasicBlock.cpp

1387

@myhsu Have you confirmed whether this can happen? It looks like it at least needs an assert, else an early-out.

[myhsu]

• Rebase to latest changes
• Addressed feedbacks

llvm/lib/CodeGen/MachineBasicBlock.cpp

1387

sorry I missed this comment. I've just fixed it.

[rengolin]

This looks good to me but I'll let other people review again and approve. Thanks!

[RKSimon]

LGTM - cheers

[jrtc27]

llvm/include/llvm/CodeGen/TargetInstrInfo.h

947–955

956

Current name doesn't make sense; the operand is PC-relative, but the question is whether that's a legal thing to have constructed.

[myhsu]

• [NFC] Addressed feedbacks

[jrtc27]

Minor whitespace issues to fix otherwise fine by me.

llvm/include/llvm/CodeGen/MachineBasicBlock.h

855

Needs a blank line

870

Needs a blank line

llvm/lib/CodeGen/MachineBasicBlock.cpp

1367

Needs a blank line

1385

Needs a blank line

[myhsu]

• [NFC] Addressed feedbacks

llvm/include/llvm/CodeGen/MachineBasicBlock.h

#endif
   void replacePhiUsesWith(MachineBasicBlock *Old, MachineBasicBlock *New);
 
   /// Find the next valid DebugLoc starting at MBBI, skipping any DBG_VALUE
   /// and DBG_LABEL instructions.  Return UnknownLoc if there is none.
   DebugLoc findDebugLoc(instr_iterator MBBI);
   DebugLoc findDebugLoc(iterator MBBI) {
     return findDebugLoc(MBBI.getInstrIterator());
   }
 
+  /// Has exact same behavior as @ref findDebugLoc (it also
+  /// searches from the first to the last MI of this MBB) except
+  /// that this takes reverse iterator.
+  DebugLoc rfindDebugLoc(reverse_instr_iterator MBBI);
+  DebugLoc rfindDebugLoc(reverse_iterator MBBI) {
+    return rfindDebugLoc(MBBI.getInstrIterator());
+  }
+
   /// Find the previous valid DebugLoc preceding MBBI, skipping and DBG_VALUE
   /// instructions.  Return UnknownLoc if there is none.
   DebugLoc findPrevDebugLoc(instr_iterator MBBI);
   DebugLoc findPrevDebugLoc(iterator MBBI) {
     return findPrevDebugLoc(MBBI.getInstrIterator());
   }
 
+  /// Has exact same behavior as @ref findPrevDebugLoc (it also
+  /// searches from the last to the first MI of this MBB) except
+  /// that this takes reverse iterator.
+  DebugLoc rfindPrevDebugLoc(reverse_instr_iterator MBBI);
+  DebugLoc rfindPrevDebugLoc(reverse_iterator MBBI) {
+    return rfindPrevDebugLoc(MBBI.getInstrIterator());
+  }
+
   /// Find and return the merged DebugLoc of the branch instructions of the
   /// block. Return UnknownLoc if there is none.
   DebugLoc findBranchDebugLoc();
 
   /// Possible outcome of a register liveness query to computeRegisterLiveness()
   enum LivenessQueryResult {
     LQR_Live,   ///< Register is known to be (at least partially) live.
     LQR_Dead,   ///< Register is known to be fully dead.

llvm/include/llvm/CodeGen/TargetInstrInfo.h

public:
   /// large registers. See for example the ARM target.
   virtual void copyPhysReg(MachineBasicBlock &MBB,
                            MachineBasicBlock::iterator MI, const DebugLoc &DL,
                            MCRegister DestReg, MCRegister SrcReg,
                            bool KillSrc) const {
     llvm_unreachable("Target didn't implement TargetInstrInfo::copyPhysReg!");
   }
 
+  /// Allow targets to tell MachineVerifier whether a specific register
+  /// MachineOperand can be used as part of PC-relative addressing.
+  /// PC-relative addressing modes in many CISC architectures contain
+  /// (non-PC) registers as offsets or scaling values, which inherently
+  /// tags the corresponding MachineOperand with OPERAND_PCREL.
+  ///
+  /// @param MO The MachineOperand in question. MO.isReg() should always
+  /// be true.
+  /// @return Whether this operand is allowed to be used PC-relatively.
+  virtual bool isPCRelRegisterOperandLegal(const MachineOperand &MO) const {
+    return false;
+  }
+
 protected:
   /// Target-dependent implementation for IsCopyInstr.
   /// If the specific machine instruction is a instruction that moves/copies
   /// value from one register to another register return destination and source
   /// registers as machine operands.
   virtual Optional<DestSourcePair>
   isCopyInstrImpl(const MachineInstr &MI) const {
     return None;

llvm/lib/CodeGen/MachineBasicBlock.cpp

 DebugLoc
 MachineBasicBlock::findDebugLoc(instr_iterator MBBI) {
   // Skip debug declarations, we don't want a DebugLoc from them.
   MBBI = skipDebugInstructionsForward(MBBI, instr_end());
   if (MBBI != instr_end())
     return MBBI->getDebugLoc();
   return {};
 }
 
+DebugLoc MachineBasicBlock::rfindDebugLoc(reverse_instr_iterator MBBI) {
+  // Skip debug declarations, we don't want a DebugLoc from them.
+  MBBI = skipDebugInstructionsBackward(MBBI, instr_rbegin());
+  if (!MBBI->isDebugInstr())
+    return MBBI->getDebugLoc();
+  return {};
+}
+
 /// Find the previous valid DebugLoc preceding MBBI, skipping and DBG_VALUE
 /// instructions.  Return UnknownLoc if there is none.
 DebugLoc MachineBasicBlock::findPrevDebugLoc(instr_iterator MBBI) {
   if (MBBI == instr_begin()) return {};
   // Skip debug instructions, we don't want a DebugLoc from them.
   MBBI = prev_nodbg(MBBI, instr_begin());
   if (!MBBI->isDebugInstr()) return MBBI->getDebugLoc();
   return {};
 }
 
+DebugLoc MachineBasicBlock::rfindPrevDebugLoc(reverse_instr_iterator MBBI) {
+  if (MBBI == instr_rend())
+    return {};
+  // Skip debug declarations, we don't want a DebugLoc from them.
+  MBBI = next_nodbg(MBBI, instr_rend());
+  if (MBBI != instr_rend())
+    return MBBI->getDebugLoc();
+  return {};
+}
+
 /// Find and return the merged DebugLoc of the branch instructions of the block.
 /// Return UnknownLoc if there is none.
 DebugLoc
 MachineBasicBlock::findBranchDebugLoc() {
   DebugLoc DL;
   auto TI = getFirstTerminator();
   while (TI != end() && !TI->isBranch())
     ++TI;

llvm/lib/CodeGen/MachineVerifier.cpp

if (!IsOptional) {
           report("Explicit operand marked as def", MO, MONum);
         if (MO->isImplicit())
           report("Explicit operand marked as implicit", MO, MONum);
       }
 
       // Check that an instruction has register operands only as expected.
       if (MCOI.OperandType == MCOI::OPERAND_REGISTER &&
           !MO->isReg() && !MO->isFI())
         report("Expected a register operand.", MO, MONum);
-      if ((MCOI.OperandType == MCOI::OPERAND_IMMEDIATE ||
-           MCOI.OperandType == MCOI::OPERAND_PCREL) && MO->isReg())
+      if (MO->isReg()) {
+        if (MCOI.OperandType == MCOI::OPERAND_IMMEDIATE ||
+            (MCOI.OperandType == MCOI::OPERAND_PCREL &&
+             !TII->isPCRelRegisterOperandLegal(*MO)))
           report("Expected a non-register operand.", MO, MONum);
       }
+    }
 
     int TiedTo = MCID.getOperandConstraint(MONum, MCOI::TIED_TO);
     if (TiedTo != -1) {
       if (!MO->isReg())
         report("Tied use must be a register", MO, MONum);
       else if (!MO->isTied())
         report("Operand should be tied", MO, MONum);
       else if (unsigned(TiedTo) != MI->findTiedOperandIdx(MONum))