Add new MachineFunction property FailsVerification

[foad]

Summary

TargetPassConfig::addPass takes a "bool verifyAfter" argument which lets
you skip machine verification after a particular pass. Unfortunately
this is used in generic code in TargetPassConfig itself to skip
verification after a generic pass, only because some previous target-
specific pass damaged the MIR on that specific target. This is bad
because problems in one target cause lack of verification for all
targets.

This patch replaces that mechanism with a new MachineFunction property
called "FailsVerification" which can be set by (usually target-specific)
passes that are known to introduce problems. Later passes can reset it
again if they are known to clean up the previous problems.

[foad]

This is an RFC to see if people like the idea.

Known issues:

1. I'm not sure if the new "Verifies" property needs to be serialised in .mir files.
2. When you look at the TargetMachine class for a particular target, it's not very easy to see where the "Verifies" flag gets turned off and on -- you have to look into the individual passes. It would be easy to have it turned off at one point in the middle of the pass pipeline, and forget to turn it on again later.

[MatzeB]

As discussed in https://reviews.llvm.org/D111386 I have a feeling that some targets can likely be improved by moving code from addPreEmitPass to addPreEmitPass2. But we should probably discuss this independently of this patch, since I have a feeling that one way or another we won't end up with 100% of things fixed.

I am in favor of this change and just have some nitpicks.

llvm/include/llvm/CodeGen/MachineFunction.h

152–166

Can we use a flag where the default/desired state is not set?

llvm/lib/CodeGen/MachineFunction.cpp

160

See above (when switching to DisableVerification).

llvm/lib/CodeGen/MachineVerifier.cpp

296–297

FWIW: I don't have high hopes for this flag ever disappearing once we introduce it and would tend to drop the comment (but no strong opinion you can keep it if you want).

llvm/lib/Target/AMDGPU/AMDILCFGStructurizer.cpp

130

So, there's this thing I've seen other projects do, where they file tickets in the bugtracker and reference those in code. I know we don't usually do this in LLVM yet, but maybe we can set a new precedent here, what do you think?

llvm/lib/Target/AMDGPU/SILowerControlFlow.cpp

797

As above.

llvm/lib/Target/Hexagon/HexagonVLIWPacketizer.cpp

206

as above

[MatzeB]

In D111397#3050710, @foad wrote:

This is an RFC to see if people like the idea.

Known issues:

1. I'm not sure if the new "Verifies" property needs to be serialised in .mir files.

Yes I think so.

2. When you look at the TargetMachine class for a particular target, it's not very easy to see where the "Verifies" flag gets turned off and on -- you have to look into the individual passes. It would be easy to have it turned off at one point in the middle of the pass pipeline, and forget to turn it on again later.

Well, I don't think we have a better solution. Regardless it's better than what we currently have.

[foad]

Invert the sense of the attribute. Split the patch into two.

[foad]

In D111397#3058959, @MatzeB wrote:

In D111397#3050710, @foad wrote:

Known issues:

1. I'm not sure if the new "Verifies" property needs to be serialised in .mir files.

Yes I think so.

Done.

llvm/lib/CodeGen/MachineVerifier.cpp

296–297

I'm an optimist!

llvm/lib/Target/AMDGPU/AMDILCFGStructurizer.cpp

130

I'm happy to do this in principle.

[MatzeB]

LGTM

[foad]

llvm/lib/Target/AMDGPU/AMDILCFGStructurizer.cpp

130

Done for the AMDGPU SILowerControlFlow case: a129932b0d45949a884cee90726bf90217c2e737
For the Hexagon packetizer case, it seems like there's more debate over whether there's really a "bug" and how it could be "fixed".

[arsenm]

llvm/include/llvm/CodeGen/MIRYamlMapping.h

726

This should not need explicit serialization. The verifier runs after mir parsing anyway.

It should also be illegal to clear this after it’s set

[foad]

llvm/include/llvm/CodeGen/MIRYamlMapping.h

726

This should not need explicit serialization.

Why? It means "this function has known problems that I haven't fixed yet so please don't run the verifier on it".

It should also be illegal to clear this after it’s set

Why?

[arsenm]

llvm/include/llvm/CodeGen/MIRYamlMapping.h

726

Oh, I misread the intent of this change. I assumed this was solving a problem I've run into where a pass introduces a compilation failure, and as a result ends up in some invalid state where the verifier crashes. For example this happens when the register allocator fails and emits an error, but the compilation still needs to continue.

I don't love the idea of allowing functions to fail the verifier, and then be fixed up later. Once the verifier fails a function should be dead.

[MatzeB]

llvm/include/llvm/CodeGen/MIRYamlMapping.h

726

Yeah, FailsVerifier is an intentional decision by pass authors, that they are aware that the pass produces MIR that will not pass the verifier.

I am pretty sure everyone would be happier to just force every machine function to very, but it's a practical concern to get to that state. Before this change we had verification completely disable for a number of passes, with this change at least we can enable verification for all passes in general and the instances where this isn't the case target authors can work around by setting the FailsVerification flag and fixing things on their own pace...

llvm/include/llvm/CodeGen/MIRYamlMapping.h

struct MachineFunction {
   // GISel MachineFunctionProperties.
   bool Legalized = false;
   bool RegBankSelected = false;
   bool Selected = false;
   bool FailedISel = false;
   // Register information
   bool TracksRegLiveness = false;
   bool HasWinCFI = false;
+  bool FailsVerification = false;
   std::vector<VirtualRegisterDefinition> VirtualRegisters;
   std::vector<MachineFunctionLiveIn> LiveIns;
   Optional<std::vector<FlowStringValue>> CalleeSavedRegisters;
   // TODO: Serialize the various register masks.
   // Frame information
   MachineFrameInfo FrameInfo;
   std::vector<FixedMachineStackObject> FixedStackObjects;
   std::vector<MachineStackObject> StackObjects;
static void mapping(IO &YamlIO, MachineFunction &MF) {
     YamlIO.mapOptional("alignment", MF.Alignment, None);
     YamlIO.mapOptional("exposesReturnsTwice", MF.ExposesReturnsTwice, false);
     YamlIO.mapOptional("legalized", MF.Legalized, false);
     YamlIO.mapOptional("regBankSelected", MF.RegBankSelected, false);
     YamlIO.mapOptional("selected", MF.Selected, false);
     YamlIO.mapOptional("failedISel", MF.FailedISel, false);
     YamlIO.mapOptional("tracksRegLiveness", MF.TracksRegLiveness, false);
     YamlIO.mapOptional("hasWinCFI", MF.HasWinCFI, false);
+    YamlIO.mapOptional("failsVerification", MF.FailsVerification, false);
     YamlIO.mapOptional("registers", MF.VirtualRegisters,
                        std::vector<VirtualRegisterDefinition>());
     YamlIO.mapOptional("liveins", MF.LiveIns,
                        std::vector<MachineFunctionLiveIn>());
     YamlIO.mapOptional("calleeSavedRegisters", MF.CalleeSavedRegisters,
                        Optional<std::vector<FlowStringValue>>());
     YamlIO.mapOptional("frameInfo", MF.FrameInfo, MachineFrameInfo());
     YamlIO.mapOptional("fixedStack", MF.FixedStackObjects,

llvm/include/llvm/CodeGen/MachineFunction.h

public:
   //  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.
   // TiedOpsRewritten: The twoaddressinstruction pass will set this flag, it
   //  means that tied-def have been rewritten to meet the RegConstraint.
+  // FailsVerification: Means that the function is not expected to pass machine
+  //  verification. This can be set by passes that introduce known problems that
+  //  have not been fixed yet.
   enum class Property : unsigned {
     IsSSA,
     NoPHIs,
     TracksLiveness,
     NoVRegs,
     FailedISel,
     Legalized,
     RegBankSelected,
     Selected,
     TiedOpsRewritten,
-    LastProperty = TiedOpsRewritten,
+    FailsVerification,
+    LastProperty = FailsVerification,
   };
 
   bool hasProperty(Property P) const {
     return Properties[static_cast<unsigned>(P)];
   }
 
   MachineFunctionProperties &set(Property P) {
     Properties.set(static_cast<unsigned>(P));

llvm/lib/CodeGen/MIRParser/MIRParser.cpp

if (YamlMF.Legalized)
     MF.getProperties().set(MachineFunctionProperties::Property::Legalized);
   if (YamlMF.RegBankSelected)
     MF.getProperties().set(
         MachineFunctionProperties::Property::RegBankSelected);
   if (YamlMF.Selected)
     MF.getProperties().set(MachineFunctionProperties::Property::Selected);
   if (YamlMF.FailedISel)
     MF.getProperties().set(MachineFunctionProperties::Property::FailedISel);
+  if (YamlMF.FailsVerification)
+    MF.getProperties().set(
+        MachineFunctionProperties::Property::FailsVerification);
 
   PerFunctionMIParsingState PFS(MF, SM, IRSlots, *Target);
   if (parseRegisterInfo(PFS, YamlMF))
     return true;
   if (!YamlMF.Constants.empty()) {
     auto *ConstantPool = MF.getConstantPool();
     assert(ConstantPool && "Constant pool must be created");
     if (initializeConstantPool(PFS, *ConstantPool, YamlMF))

llvm/lib/CodeGen/MIRPrinter.cpp

void MIRPrinter::print(const MachineFunction &MF) {
   YamlMF.Legalized = MF.getProperties().hasProperty(
       MachineFunctionProperties::Property::Legalized);
   YamlMF.RegBankSelected = MF.getProperties().hasProperty(
       MachineFunctionProperties::Property::RegBankSelected);
   YamlMF.Selected = MF.getProperties().hasProperty(
       MachineFunctionProperties::Property::Selected);
   YamlMF.FailedISel = MF.getProperties().hasProperty(
       MachineFunctionProperties::Property::FailedISel);
+  YamlMF.FailsVerification = MF.getProperties().hasProperty(
+      MachineFunctionProperties::Property::FailsVerification);
 
   convert(YamlMF, MF.getRegInfo(), MF.getSubtarget().getRegisterInfo());
   MachineModuleSlotTracker MST(&MF);
   MST.incorporateFunction(MF.getFunction());
   convert(MST, YamlMF.FrameInfo, MF.getFrameInfo());
   convertStackObjects(YamlMF, MF, MST);
   convertCallSiteObjects(YamlMF, MF, MST);
   for (const auto &Sub : MF.DebugValueSubstitutions) {

llvm/lib/CodeGen/MachineFunction.cpp

static const char *getPropertyName(MachineFunctionProperties::Property Prop) {
   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::TiedOpsRewritten: return "TiedOpsRewritten";
+  case P::FailsVerification: return "FailsVerification";
   }
   llvm_unreachable("Invalid machine function property");
 }
 
 // Pin the vtable to this file.
 void MachineFunction::Delegate::anchor() {}
 
 void MachineFunctionProperties::print(raw_ostream &OS) const {
void MachineFunction::handleRemoval(MachineInstr &MI) {
   if (TheDelegate)
     TheDelegate->MF_HandleRemoval(MI);
 }
 
 void MachineFunction::init() {
   // Assume the function starts in SSA form with correct liveness.
   Properties.set(MachineFunctionProperties::Property::IsSSA);
   Properties.set(MachineFunctionProperties::Property::TracksLiveness);
   if (STI->getRegisterInfo())
     RegInfo = new (Allocator) MachineRegisterInfo(this);
   else
     RegInfo = nullptr;
 
   MFInfo = nullptr;
   // We can realign the stack if the target supports it and the user hasn't
   // explicitly asked us not to.
   bool CanRealignSP = STI->getFrameLowering()->isStackRealignable() &&

llvm/lib/CodeGen/MachineVerifier.cpp

MachineVerifierPass(std::string banner = std::string())
       }
 
     void getAnalysisUsage(AnalysisUsage &AU) const override {
       AU.setPreservesAll();
       MachineFunctionPass::getAnalysisUsage(AU);
     }
 
     bool runOnMachineFunction(MachineFunction &MF) override {
+      // Skip functions that have known verification problems.
+      // FIXME: Remove this mechanism when all problematic passes have been
+      // fixed.
+      if (MF.getProperties().hasProperty(
+              MachineFunctionProperties::Property::FailsVerification))
+        return false;
+
       unsigned FoundErrors = MachineVerifier(this, Banner.c_str()).verify(MF);
       if (FoundErrors)
         report_fatal_error("Found "+Twine(FoundErrors)+" machine code errors.");
       return false;
     }
   };
 
 } // end anonymous namespace

llvm/lib/Target/AMDGPU/AMDILCFGStructurizer.cpp

public:
   bool run();
 
   /// Perform the CFG preparation
   /// This step will remove every unconditionnal/dead jump instructions and make
   /// sure all loops have an exit block
   bool prepare();
 
   bool runOnMachineFunction(MachineFunction &MF) override {
+    // FIXME: This pass causes verification failures.
+    MF.getProperties().set(
+        MachineFunctionProperties::Property::FailsVerification);
+
     TII = MF.getSubtarget<R600Subtarget>().getInstrInfo();
     TRI = &TII->getRegisterInfo();
     LLVM_DEBUG(MF.dump(););
     OrderedBlks.clear();
     Visited.clear();
     FuncRep = &MF;
     MLI = &getAnalysis<MachineLoopInfo>();
     LLVM_DEBUG(dbgs() << "LoopInfo:\n"; PrintLoopinfo(*MLI););

llvm/lib/Target/AMDGPU/SILowerControlFlow.cpp

if (!GetFallThroughSucc(Succ)) {
               FallThrough->findBranchDebugLoc(), TII->get(AMDGPU::S_BRANCH))
           .addMBB(Succ);
   }
 
   return true;
 }
 
 bool SILowerControlFlow::runOnMachineFunction(MachineFunction &MF) {
+  // FIXME: This pass causes verification failures.
+  MF.getProperties().set(
+      MachineFunctionProperties::Property::FailsVerification);
+
   const GCNSubtarget &ST = MF.getSubtarget<GCNSubtarget>();
   TII = ST.getInstrInfo();
   TRI = &TII->getRegisterInfo();
 
   // This doesn't actually need LiveIntervals, but we can preserve them.
   LIS = getAnalysisIfAvailable<LiveIntervals>();
   MDT = getAnalysisIfAvailable<MachineDominatorTree>();
   MRI = &MF.getRegInfo();

llvm/lib/Target/AMDGPU/SIWholeQuadMode.cpp

default:
       continue;
     }
     if (SplitPoint)
       splitBlock(MBB, SplitPoint);
   }
 }
 
 bool SIWholeQuadMode::runOnMachineFunction(MachineFunction &MF) {
+  // This pass is a convenient place to re-enable machine verification after the
+  // problems caused by SILowerControlFlow have been fixed.
+  MF.getProperties().reset(
+      MachineFunctionProperties::Property::FailsVerification);
+
   LLVM_DEBUG(dbgs() << "SI Whole Quad Mode on " << MF.getName()
                     << " ------------- \n");
   LLVM_DEBUG(MF.dump(););
 
   Instructions.clear();
   Blocks.clear();
   LiveMaskQueries.clear();
   LowerToCopyInstrs.clear();

llvm/lib/Target/Hexagon/HexagonVLIWPacketizer.cpp

static MachineBasicBlock::iterator moveInstrOut(MachineInstr &MI,
   MachineInstr &SingleI = *BundleIt->getNextNode();
   SingleI.unbundleFromPred();
   assert(!SingleI.isBundledWithSucc());
   BundleIt->eraseFromParent();
   return NextIt;
 }
 
 bool HexagonPacketizer::runOnMachineFunction(MachineFunction &MF) {
+  // FIXME: This pass causes verification failures.
+  MF.getProperties().set(
+      MachineFunctionProperties::Property::FailsVerification);
+
   auto &HST = MF.getSubtarget<HexagonSubtarget>();
   HII = HST.getInstrInfo();
   HRI = HST.getRegisterInfo();
   auto &MLI = getAnalysis<MachineLoopInfo>();
   auto *AA = &getAnalysis<AAResultsWrapperPass>().getAAResults();
   auto *MBPI = &getAnalysis<MachineBranchProbabilityInfo>();
 
   if (EnableGenAllInsnClass)