[MachineVerifier] Add a new TiedOpsRewritten flag to fix verify two-address constraint error

[ZhangKang]

Summary

Currently, MachineVerifier will attempt to verify that tied operands satisfy register constraints
as soon as the function is no longer in SSA form. However, PHIElimination will take the
function out of SSA form while TwoAddressInstructionPass will actually rewrite tied operands
to match the constraints. PHIElimination runs first in the pipeline.
Therefore, whenever the MachineVerifier is run after PHIElimination, it will encounter
verification errors on any tied operands.

This patch adds a function property called TiedOpsRewritten that will be set by
TwoAddressInstructionPass and will control when the verifier checks tied operands.

This patch can fix 4752 cases error for two-address constraint verification.

[ZhangKang]

The TwoAddressInstructionPass will rewrite the tied to meet the RegConstraint.
For example:
The TwoAddressInstructionPass will rewrite below IR

%10:gprc = RLWIMI killed %9:gprc(tied-def 0), killed %0:gprc, 1, 0, 30

to:

%10:gprc = RLWIMI killed %10:gprc(tied-def 0), killed %0:gprc, 1, 0, 30

[nemanjai]

Just to be clear, I don't actually have an issue with the code changes. The reason I am requesting changes is to ensure that the description (and presumably commit message) adequately describes the problem being solved.

llvm/include/llvm/CodeGen/MachineFunction.h

158

I think a more descriptive name would be something like TiedOpsRewritten.

llvm/lib/CodeGen/MachineVerifier.cpp

1659

The description does not adequately describe why this is needed. Namely, we will set this property at the same point in the code as the call to MRI->leaveSSA(). So how do the following two checks differ:

MF->getProperties().hasProperty(
    MachineFunctionProperties::Property::RewriteTied)

vs.
!MRI->isSSA()

It would appear that the issue is when we run the verifier after PHI elimination and not after Two Address.

llvm/test/CodeGen/PowerPC/two-address-crash.mir

1

I think it would be more useful to -start-before=phi-node-elimination and thereby test the verification after all the passes so that verification of the tied operands is actually performed.

[ZhangKang]

llvm/include/llvm/CodeGen/MachineFunction.h

158

Done.

llvm/lib/CodeGen/MachineVerifier.cpp

1659

Have modified the comment.

llvm/test/CodeGen/PowerPC/two-address-crash.mir

1

In fact, if I use -start-before=phi-node-elimination, whether using my patch, there is no error, because the verification for phi-node-elimination pass and twoaddressinstruction pass have been disabled, that why we haven't gotten any error before.
But as you said, I still add -start-before=phi-node-elimination, because it can test the passes which have enabled verification.

[ZhangKang]

Modify the comments and test case.

[craig.topper]

llvm/include/llvm/CodeGen/MachineFunction.h

147

Update comment to match the new enum name.

[ZhangKang]

Update the comments.

[ZhangKang]

llvm/include/llvm/CodeGen/MachineFunction.h

147

Done.

[lkail]

The title should be also rephrased.

[nemanjai]

The following sentence in the description is still incorrect:

We should verify two-address constraints after TwoAddressInstructionPass pass has been run, But in the MachineVerifier.cpp, we will
verify two-address constraints when the machine function is SSA, this is not correct.

I think it suffices for the description to just succinctly describe the problem. Something like:

Currently MachineVerifier will attempt to verify that tied operands satisfy register constraints
as soon as the function is no longer in SSA form. However, PHIElimination will take the
function out of SSA form while TwoAddressInstructionPass will actually rewrite tied operands
to match the constraints. PHIElimination runs first in the pipeline.
Therefore whenever the MachineVerifier is run after PHIElimination, it will encounter
verification errors on any tied operands.

This patch adds a function property called TiedOpsRewritten that will be set by 
TwoAddressInstructionPass and will control when the verifier checks tied operands.

Other than that, LGTM.

[MatzeB]

llvm/include/llvm/CodeGen/MachineFunction.h

158–159

You have to add parsing support for .mir files when adding new function properties. See MIRYamlMapping.h / MIRParser.cpp

[shchenz]

llvm/include/llvm/CodeGen/MachineFunction.h

158–159

Thanks. We will address this in a follow-up patch.

llvm/include/llvm/CodeGen/MachineFunction.h

public:
   //  virtual registers have been assigned to a register bank.
   // Selected: In GlobalISel: the InstructionSelect pass ran and all pre-isel
   //  generic instructions have been eliminated; i.e., all instructions are now
   //  target-specific or non-pre-isel generic instructions (e.g., COPY).
   //  Since only pre-isel generic instructions can have generic virtual register
   //  operands, this also means that all generic virtual registers have been
   //  constrained to virtual registers (assigned to register classes) and that
   //  all sizes attached to them have been eliminated.
+  // RewriteTied: The twoaddressinstruction pass will set this flag, it means
+  //  that tied-def have been rewritten to meet the RegConstraint.
   enum class Property : unsigned {
     IsSSA,
     NoPHIs,
     TracksLiveness,
     NoVRegs,
     FailedISel,
     Legalized,
     RegBankSelected,
     Selected,
-    LastProperty = Selected,
+    RewriteTied,
+    LastProperty = RewriteTied,
   };
 
   bool hasProperty(Property P) const {
     return Properties[static_cast<unsigned>(P)];
   }
 
   MachineFunctionProperties &set(Property P) {
     Properties.set(static_cast<unsigned>(P));

llvm/lib/CodeGen/MachineFunction.cpp

static const char *getPropertyName(MachineFunctionProperties::Property Prop) {
   case P::FailedISel: return "FailedISel";
   case P::IsSSA: return "IsSSA";
   case P::Legalized: return "Legalized";
   case P::NoPHIs: return "NoPHIs";
   case P::NoVRegs: return "NoVRegs";
   case P::RegBankSelected: return "RegBankSelected";
   case P::Selected: return "Selected";
   case P::TracksLiveness: return "TracksLiveness";
+  case P::RewriteTied: return "RewriteTied";
   }
   llvm_unreachable("Invalid machine function property");
 }
 
 // Pin the vtable to this file.
 void MachineFunction::Delegate::anchor() {}
 
 void MachineFunctionProperties::print(raw_ostream &OS) const {

llvm/lib/CodeGen/MachineVerifier.cpp

if (MO->isTied()) {
                    MO, MONum);
         } else {
           if (!OtherMO.isImplicit())
             report("Explicit def should be tied to implicit use", MO, MONum);
         }
       }
     }
 
-    // Verify two-address constraints after leaving SSA form.
+    // Verify two-address constraints after the twoaddressinstruction pass.
     unsigned DefIdx;
-    if (!MRI->isSSA() && MO->isUse() &&
-        MI->isRegTiedToDefOperand(MONum, &DefIdx) &&
+    if (MF->getProperties().hasProperty(
+            MachineFunctionProperties::Property::RewriteTied) &&
+        MO->isUse() && MI->isRegTiedToDefOperand(MONum, &DefIdx) &&
         Reg != MI->getOperand(DefIdx).getReg())
       report("Two-address instruction operands must be identical", MO, MONum);
 
     // Check register classes.
     unsigned SubIdx = MO->getSubReg();
 
     if (Register::isPhysicalRegister(Reg)) {
       if (SubIdx) {

llvm/lib/CodeGen/TwoAddressInstructionPass.cpp

bool TwoAddressInstructionPass::runOnMachineFunction(MachineFunction &Func) {
   bool MadeChange = false;
 
   LLVM_DEBUG(dbgs() << "********** REWRITING TWO-ADDR INSTRS **********\n");
   LLVM_DEBUG(dbgs() << "********** Function: " << MF->getName() << '\n');
 
   // This pass takes the function out of SSA form.
   MRI->leaveSSA();
 
+  // This pass will rewrite the tied-def to meet the RegConstraint.
+  MF->getProperties().set(MachineFunctionProperties::Property::RewriteTied);
+
   TiedOperandMap TiedOperands;
   for (MachineFunction::iterator MBBI = MF->begin(), MBBE = MF->end();
        MBBI != MBBE; ++MBBI) {
     MBB = &*MBBI;
     unsigned Dist = 0;
     DistanceMap.clear();
     SrcRegMap.clear();
     DstRegMap.clear();

llvm/test/CodeGen/PowerPC/two-address-crash.mir

-# RUN: not --crash llc -mtriple=ppc32-- %s -run-pass=phi-node-elimination \
-# RUN:   -verify-machineinstrs -o /dev/null 2>&1 | FileCheck %s
+# RUN: llc -mtriple=ppc32-- %s -run-pass=phi-node-elimination \
+# RUN:   -verify-machineinstrs -o /dev/null 2>&1
 
 --- |
   define void @VerifyTwoAddressCrash(i16 %div.0.i.i.i.i, i32 %L_num.0.i.i.i.i, i32 %tmp1.i.i206.i.i, i16* %P) {
     %X = shl i16 %div.0.i.i.i.i, 1
     %tmp28.i.i.i.i = shl i32 %L_num.0.i.i.i.i, 1
     %tmp31.i.i.i.i = icmp slt i32 %tmp28.i.i.i.i, %tmp1.i.i206.i.i
     %tmp31.i.i.i.i.upgrd.1 = zext i1 %tmp31.i.i.i.i to i16
     %tmp371.i.i.i.i1 = or i16 %tmp31.i.i.i.i.upgrd.1, %X
bb.0 (%ir-block.0):
     %8:gprc_and_gprc_nor0 = LI 1
     %9:gprc = ISEL killed %8, killed %7, killed %6
     %10:gprc = RLWIMI killed %9, killed %0, 1, 0, 30
     %11:gprc = XORI killed %10, 1
     STH killed %11, 0, killed %3 :: (store 2 into %ir.P)
     BLR implicit $lr, implicit $rm
 
 ...
-
-# CHECK-LABEL: Bad machine code: Two-address instruction operands must be identical
-# CHECK-NEXT:  - function:    VerifyTwoAddressCrash
-# CHECK-NEXT:  - basic block: %bb.0
-# CHECK-NEXT:  - instruction: %10:gprc = RLWIMI killed %9:gprc(tied-def 0), killed %3:gprc, 1, 0, 30
-# CHECK-NEXT:  - operand 1:   killed %9:gprc(tied-def 0)
-# CHECK-NEXT:  LLVM ERROR: Found 1 machine code errors.
+# Used to result in
+#
+# Bad machine code: Two-address instruction operands must be identical
+# - function:    VerifyTwoAddressCrash
+# - basic block: %bb.0
+# - instruction: %10:gprc = RLWIMI killed %9:gprc(tied-def 0), killed %3:gprc, 1, 0, 30
+# - operand 1:   killed %9:gprc(tied-def 0)
+# LLVM ERROR: Found 1 machine code errors.
+#
+# Just verify that we do not crash (or get verifier error).