[llvm] 1b66022 - [MachineVerifier] Add TiedOpsRewritten flag to fix verify two-address error

[Kang Zhang via llvm-commits]

Author: Kang Zhang
Date: 2020-06-09T07:39:42Z
New Revision: 1b6602275d3f902a91e2eab28f2ac506372d9065

URL: https://github.com/llvm/llvm-project/commit/1b6602275d3f902a91e2eab28f2ac506372d9065
DIFF: https://github.com/llvm/llvm-project/commit/1b6602275d3f902a91e2eab28f2ac506372d9065.diff

LOG: [MachineVerifier] Add TiedOpsRewritten flag to fix verify two-address error

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.

Reviewed By: nemanjai

Differential Revision: https://reviews.llvm.org/D80538

Added: 
    

Modified: 
    llvm/include/llvm/CodeGen/MachineFunction.h
    llvm/lib/CodeGen/MachineFunction.cpp
    llvm/lib/CodeGen/MachineVerifier.cpp
    llvm/lib/CodeGen/TwoAddressInstructionPass.cpp
    llvm/test/CodeGen/PowerPC/two-address-crash.mir

Removed: 
    


################################################################################
diff  --git a/llvm/include/llvm/CodeGen/MachineFunction.h b/llvm/include/llvm/CodeGen/MachineFunction.h
index d77d8652f3d0..809c21dd26fc 100644
--- a/llvm/include/llvm/CodeGen/MachineFunction.h
+++ b/llvm/include/llvm/CodeGen/MachineFunction.h
@@ -144,6 +144,8 @@ class MachineFunctionProperties {
   //  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.
+  // TiedOpsRewritten: 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,
@@ -153,7 +155,8 @@ class MachineFunctionProperties {
     Legalized,
     RegBankSelected,
     Selected,
-    LastProperty = Selected,
+    TiedOpsRewritten,
+    LastProperty = TiedOpsRewritten,
   };
 
   bool hasProperty(Property P) const {

diff  --git a/llvm/lib/CodeGen/MachineFunction.cpp b/llvm/lib/CodeGen/MachineFunction.cpp
index 3e57e3c38932..6d45f08804ed 100644
--- a/llvm/lib/CodeGen/MachineFunction.cpp
+++ b/llvm/lib/CodeGen/MachineFunction.cpp
@@ -98,6 +98,7 @@ static const char *getPropertyName(MachineFunctionProperties::Property Prop) {
   case P::RegBankSelected: return "RegBankSelected";
   case P::Selected: return "Selected";
   case P::TracksLiveness: return "TracksLiveness";
+  case P::TiedOpsRewritten: return "TiedOpsRewritten";
   }
   llvm_unreachable("Invalid machine function property");
 }

diff  --git a/llvm/lib/CodeGen/MachineVerifier.cpp b/llvm/lib/CodeGen/MachineVerifier.cpp
index d0568b35505e..df23ccf4e195 100644
--- a/llvm/lib/CodeGen/MachineVerifier.cpp
+++ b/llvm/lib/CodeGen/MachineVerifier.cpp
@@ -1633,10 +1633,17 @@ MachineVerifier::visitMachineOperand(const MachineOperand *MO, unsigned MONum) {
       }
     }
 
-    // Verify two-address constraints after leaving SSA form.
+    // Verify two-address constraints after the twoaddressinstruction pass.
+    // Both twoaddressinstruction pass and phi-node-elimination pass call
+    // MRI->leaveSSA() to set MF as NoSSA, we should do the verification after
+    // twoaddressinstruction pass not after phi-node-elimination pass. So we
+    // shouldn't use the NoSSA as the condition, we should based on
+    // TiedOpsRewritten property to verify two-address constraints, this
+    // property will be set in twoaddressinstruction pass.
     unsigned DefIdx;
-    if (!MRI->isSSA() && MO->isUse() &&
-        MI->isRegTiedToDefOperand(MONum, &DefIdx) &&
+    if (MF->getProperties().hasProperty(
+            MachineFunctionProperties::Property::TiedOpsRewritten) &&
+        MO->isUse() && MI->isRegTiedToDefOperand(MONum, &DefIdx) &&
         Reg != MI->getOperand(DefIdx).getReg())
       report("Two-address instruction operands must be identical", MO, MONum);
 

diff  --git a/llvm/lib/CodeGen/TwoAddressInstructionPass.cpp b/llvm/lib/CodeGen/TwoAddressInstructionPass.cpp
index 463311a2094a..de336abe607a 100644
--- a/llvm/lib/CodeGen/TwoAddressInstructionPass.cpp
+++ b/llvm/lib/CodeGen/TwoAddressInstructionPass.cpp
@@ -1687,6 +1687,10 @@ bool TwoAddressInstructionPass::runOnMachineFunction(MachineFunction &Func) {
   // 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::TiedOpsRewritten);
+
   TiedOperandMap TiedOperands;
   for (MachineFunction::iterator MBBI = MF->begin(), MBBE = MF->end();
        MBBI != MBBE; ++MBBI) {

diff  --git a/llvm/test/CodeGen/PowerPC/two-address-crash.mir b/llvm/test/CodeGen/PowerPC/two-address-crash.mir
index caf036358af9..d35fcb36e7ec 100644
--- a/llvm/test/CodeGen/PowerPC/two-address-crash.mir
+++ b/llvm/test/CodeGen/PowerPC/two-address-crash.mir
@@ -1,5 +1,5 @@
-# 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
 # RUN: llc -mtriple=ppc32-- %s -start-before=phi-node-elimination \
 # RUN:   -verify-machineinstrs -o /dev/null 2>&1
 
@@ -89,9 +89,12 @@ body:             |
 
 ...
 
-# 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 %0: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 %0: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).