[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

1648

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

1357

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

[rengolin]

llvm/include/llvm/CodeGen/TargetInstrInfo.h

947 ↗

(On Diff #296011)

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

[myhsu]

llvm/include/llvm/CodeGen/TargetInstrInfo.h

947 ↗

(On Diff #296011)

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

947 ↗

(On Diff #296011)

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

1357

@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

1357

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 ↗

(On Diff #310670)

956 ↗

(On Diff #310670)

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

840

Needs a blank line

851

Needs a blank line

llvm/lib/CodeGen/MachineBasicBlock.cpp

1338

Needs a blank line

1355

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());
   }
+  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());
   }
+  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.

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) {
+  // Skip debug declarations, we don't want a DebugLoc from them.
+  MBBI = skipDebugInstructionsForward(std::prev(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;
 
   if (TI != end()) {
     DL = TI->getDebugLoc();
     for (++TI ; TI != end() ; ++TI)

llvm/lib/CodeGen/MachineVerifier.cpp

if (!IsOptional) {
         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 (MCOI.OperandType == MCOI::OPERAND_IMMEDIATE && MO->isReg())
         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())