From 1b6602275d3f902a91e2eab28f2ac506372d9065 Mon Sep 17 00:00:00 2001 From: Kang Zhang Date: Tue, 9 Jun 2020 07:39:42 +0000 Subject: [PATCH] [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 --- llvm/include/llvm/CodeGen/MachineFunction.h | 5 ++++- llvm/lib/CodeGen/MachineFunction.cpp | 1 + llvm/lib/CodeGen/MachineVerifier.cpp | 13 ++++++++++--- llvm/lib/CodeGen/TwoAddressInstructionPass.cpp | 4 ++++ llvm/test/CodeGen/PowerPC/two-address-crash.mir | 19 +++++++++++-------- 5 files changed, 30 insertions(+), 12 deletions(-) diff --git a/llvm/include/llvm/CodeGen/MachineFunction.h b/llvm/include/llvm/CodeGen/MachineFunction.h index d77d865..809c21d 100644 --- a/llvm/include/llvm/CodeGen/MachineFunction.h +++ b/llvm/include/llvm/CodeGen/MachineFunction.h @@ -144,6 +144,8 @@ public: // 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 @@ public: 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 3e57e3c..6d45f08 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 d0568b3..df23ccf 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 463311a..de336ab 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 caf0363..d35fcb3 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). -- 2.7.4